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

Drop Carthage and build using Swift Package Manager (close #735) #737

Merged
merged 8 commits into from Dec 7, 2022

Conversation

matus-tomlein
Copy link
Contributor

@matus-tomlein matus-tomlein commented Dec 2, 2022

Issue #735

Dropped Carthage for build

Development and test dependencies are handled using Swift Package Manager instead of Carthage. Carthage is removed altogether, so it's not even possible to install in other apps using Carthage.

In order to do that, I had to deal with the test dependencies that were previously installed using Carthage:

  • Nocilla was used for mocking HTTP requests (for remote config or to collector). Since the project is abandoned and doesn't support SPM, I had to find a replacement. Ended up with Mocker from WeTransfer, which seems to be actively maintained. The only downside seems to be that it currently doesn't work well on watchOS but hopefully that will be addressed.
  • IgluClient was used to validate event payloads using schemas. The ObjC library is also no longer maintained and doesn't support SPM. I tried to add SPM, but there is an external dependency in the project for KiteJSONValidator, which is also not actively maintained and doesn't support SPM. Adding support for SPM there proved not easy because it contains resources (JSON schemas) that I couldn't add to there.
    • Instead of investing more time into getting the Iglu client to work on SPM, I thought that it'd be better to replace it with Micro. I think that is the more modern and scalable approach (doesn't require a separate client for each language) that we use in tracker tests. I'm happy to reconsider this if someone has a different opinion. Created a separate issue to adopt Micro

Removed XCode project files

Since SPM projects don't require an XCode project file, I removed it from the repository to clean up. XCode can work with the project just by opening the folder. If there is a need, one can regenerate the XCode project file using (but this is not recommended):

swift package generate-xcodeproj

Also cleaned up and removed a bunch of other boilerplate files that are no longer necessary.

Improved CI

Previously the CI only ran tests on iOS and built for watchOS. Now it runs tests for iOS, macOS, watchOS and tvOS. Additionally, it runs lint using CocoaPods.

Copy link
Contributor

@mscwilson mscwilson left a comment

Choose a reason for hiding this comment

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

LGTM. Great to get rid of that carthage-workaround script

Copy link
Contributor

@AlexBenny AlexBenny left a comment

Choose a reason for hiding this comment

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

LGTM!! Just a note about the dropping of Carthage support. I wonder if we should pre-announce it on Discourse (remarking the alpha version is out to be tested). Other frameworks that dropped Carthage received some criticism on GitHub so I think we should be prepared to similar requests or questions and be able to provide answers and eventually identify in advance anything that can change our mind in the process of dropping Carthage.

with:
repository: snowplow-incubator/snowplow-swift-ios-tracker-examples
repository: snowplow-incubator/snowplow-objc-tracker-examples
ref: ${{ steps.example_branch.outputs.name }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this diff but something that we should consider. I managed the relation between the examples repository and the tracker repository using the "demo_apps_branch_name" file in the scripts folder. Despite it was a huge step forward I'm not really proud of the final results. I wonder if using the submodules would have been more useful and easier to maintain. So something to consider for future improvements, also because a similar approach can be used to test we don't break anything in the wrapper trackers (React Native, Flutter).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I think submodules would be a good option to make the mapping between the repo and examples more explicit. Will have to investigate how Swift Package Manager handles submodules when including in users apps though.

Comment on lines +10 to +11
.tvOS("12.0"),
.watchOS("6.0")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think bumping them up makes sense. I wonder whether you identified any specific reason for that or it's just to drop too old versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to bump them because of the Mocker dependency used in tests which required these minimum deployment versions.

@matus-tomlein
Copy link
Contributor Author

Thanks @AlexBenny!

As we already discussed, I will post on Discourse a question along with alpha 2 to ask which package managers people use and whether they will miss Carthage.

Reintroducing Carthage would mean adding back the XCode project file and maintaining that which is a bit annoying but not such a big deal.

@matus-tomlein matus-tomlein merged commit e7328d3 into release/5.0.0-alpha.2 Dec 7, 2022
@matus-tomlein matus-tomlein deleted the issue/735-drop_carthage branch December 7, 2022 10:22
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.

None yet

3 participants