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

RUMM-1744 Copy Kronos code to SDK #701

Conversation

ncreated
Copy link
Member

What and why?

📦 This PR copies MobileNativeFoundation/Kronos@5259250 code directly to our SDK and removes the link to Kronos in all dependency managers.

There are few motivations to use our own version of Kronos:

  • We want to collect diagnostic and telemetry data from Kronos code (in our dogfood projects) to analyse and act on Kronos dependency triggers a Local Network Permission alert on iOS >= 14 #647 issue.
  • Own version gives us more control over this code - this especially important if we want to support platforms x dependency managers which are not supported in upstream Kronos.
  • Embedded code means less dependencies to our SDK users - faster installation (e.g. carthage bootstrap) and simpler linking for their app runtimes.

On the other hand, we shouldn't lose much by dropping external dependency on Kronos. It is well sustained project, solving only one problem (NTP sync) and not requiring significant updates nor bugfixes (ref.: release history). We will track all updates to the original repo with applying critical patches. Also, if we manage to find & fix issues in original implementation, we should contribute to the upstream project.

How?

  • Recent Kronos code and tests (MobileNativeFoundation/Kronos@5259250) were copied to our repository - everything was put into separate folder.
  • I removed Kronos dependency from Datadog.podspec, Cartfile and Package.swift.
  • For clarity, I prefixed all imported types and files with Kronos*.
  • Changed public access control to internal for respective types.
  • Adjusted code style to pass our linter.
  • Added licensing header in each file, referencing original authors.

Screenshot 2021-12-29 at 11 21 39

Next PR(s):

I found two problems, that will be addressed in separate PR(s), to keep this one small and clear:

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

@ncreated ncreated requested a review from a team as a code owner December 29, 2021 11:25
@ncreated ncreated self-assigned this Dec 29, 2021
Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

All good 👍

@@ -14,8 +14,6 @@
61C36425243752A600C4D4E6 /* LaunchScreen.storyboard in Resources */ = {isa = PBXBuildFile; fileRef = 61C36423243752A600C4D4E6 /* LaunchScreen.storyboard */; };
61C36430243752A600C4D4E6 /* CTProjectTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 61C3642F243752A600C4D4E6 /* CTProjectTests.swift */; };
61C3643B243752A600C4D4E6 /* CTProjectUITests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 61C3643A243752A600C4D4E6 /* CTProjectUITests.swift */; };
9E9D5E8825F90FC6002F12A0 /* Kronos.xcframework in Frameworks */ = {isa = PBXBuildFile; fileRef = 9E9D5E8525F90FC6002F12A0 /* Kronos.xcframework */; };
Copy link
Contributor

Choose a reason for hiding this comment

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

this is actually a breaking change for our carthage users.
ideally this would require a major version update.
we should at least communicate this in release notes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed 👍, Carthage users will have to remove Kronos.xcframework from their list of linked libraries, otherwise it will fail on first clean build. Although, I don't think it's a breaking change requiring major update (well, it's Carthage limitation), but it's definitely something we need to mention under "Migration Steps (Carthage)" in Release Notes for 1.9.0 👌.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added needs-migration-steps-explained label.

@ncreated ncreated merged commit d68908b into ncreated/RUMM-1744-embed-Kronos-directly-into-SDK Dec 30, 2021
@ncreated ncreated deleted the ncreated/RUMM-1744-move-Kronos-code-to-SDK branch December 30, 2021 09:00
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