Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Issue #125] Enable DatadogCore and DatadogLogs to compile on watchOS platform #1918

Conversation

jfiser-paylocity
Copy link
Contributor

What and why?

Related to issue #125.

This PR resolves compilation errors for watchOS platform in the following targets:

  • DatadogInternal
  • DatadogCore
  • DatadogLogs

How?

Minimum required version of watchOS 7 has been added to SPM/Cocoapods/Carthage definitions. The biggest issue on lower version is the lack of system notifications (didBecomeActive, didEnterBackground, willResignActive, willEnterForeground).

There are couple of limitations on watchOS platform overall, however I don’t see these as blockers:

  • No background task - Logs will still sync when the app is on the foreground.
  • HTTP header Proxy-Authorization is not set - Could be mitigated by using String static values
  • Kronos library always returns local time

To test this, you can temporarily add Apple Watch under Supported Destinations in respective targets. However it might be good to ensure at compilation time that watchOS platform does not break in the future.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

  • Run unit tests for Session Replay
  • Run smoke tests
  • Run tests for tools/

Example

I tried to integrate fork changes directly in the POC project of mine to validate it works. See below is the example log opened in web Datadog dashboard.

image

@jfiser-paylocity jfiser-paylocity requested review from a team as code owners June 21, 2024 14:04
Copy link
Member

@maciejburda maciejburda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting and valuable contribution! 🙇

It's quite complex to test this, but I'll try to schedule some time next week to give it a closer look.

For now I enabled CI run and gave it a first pass.

DatadogCore/Sources/Core/Upload/FeatureUpload.swift Outdated Show resolved Hide resolved
Copy link
Member

@maciejburda maciejburda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general it LGTM!

I think we need to consider writing a smoke test for watch kit build (the same way we do it for catalyst and macOS). With this we'll prevent from breaking the build in the future releases.

DatadogInternal/Sources/Utils/WatchKitExtensions.swift Outdated Show resolved Hide resolved
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfiser-paylocity Thank you very much for your contribution! Your effort in identifying all the necessary areas for updates is impressive and much appreciated 🏅.

We will take some time to thoroughly test these changes to ensure basics are fine and nothing crucial is missed.

In the meantime, could you please update your branch with our latest develop and conduct a round of minimal testing? It seems that some tests are currently failing:

make spm-build-ios ✅
make spm-build-tvos ✅
make spm-build-visionos ✅
make spm-build-macos ✅

make test-ios-all ❌
make test-tvos-all ❌

The failures in the last two commands (unit tests) are likely due to the omission of the two new DatadogInternal files in Datadog.xcodeproj. Including these files is crucial for building Carthage artifacts and deploying XCFrameworks from the SDK. Please address address this issue. It is is the only blocker I can see for now.

Testing failed:
	Cannot find type 'DeviceProtocol' in scope

@jfiser-paylocity
Copy link
Contributor Author

@maciejburda @ncreated Thank you for your response. All the mentioned issues should be resolved now. Please let me know if there is anything I may have missed.

@ncreated ncreated changed the base branch from develop to jfiser-paylocity/enable-watchos-build July 8, 2024 07:49
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfiser-paylocity Thanks for updates and enabling spm tests for watchOS 👌 🎯! I did more validations on our side and it looks good. Like I said before this contribution looks very exhaustively and high quality 🏅 so we should be with shipping it into our next releases.

We will proceed with this PR by first merging it to our own branch, so we can run full CI pipeline and do last polishing.

@ncreated ncreated merged commit a0b3240 into DataDog:jfiser-paylocity/enable-watchos-build Jul 8, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants