-
Notifications
You must be signed in to change notification settings - Fork 810
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
Add support for conditionally linking dependencies based on platform #1087
Add support for conditionally linking dependencies based on platform #1087
Conversation
Spitballing here: What if we changed the other field to be called
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @daltonclaybrook. I agree with the rename of platformFilter, so please proceed. It can be marked as a breaking change in the changelog.
Sources/ProjectSpec/Dependency.swift
Outdated
@@ -137,6 +144,7 @@ extension Dependency: JSONEncodable { | |||
"embed": embed, | |||
"codeSign": codeSign, | |||
"link": link, | |||
"conditionalPlatforms": conditionalPlatforms.map(Array.init)?.map(\.rawValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a sort to make this deterministic. And I also don't think the Array init is required
@yonaskolb Just pushed up those changes. How do you feel about the name of the new field (currently " |
@yonaskolb Please let me know what the next steps are on this. I'd really like to get this landed since my team needs it for an upcoming project. |
Yes, let's rename this to |
2ca3e65
to
2d8a7c0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks @daltonclaybrook!
This PR makes it possible to conditionally link dependencies based on a specified list of platforms. It addresses an issue raised in #951 that the "platform filter" parameter in Xcode is currently not granular enough to prevent a framework from linking against tvOS and watchOS targets.
This is currently implemented as a new field on
Dependency
calledconditionalPlatforms
. In all honestly, I would prefer that the existingplatform
setting be renamed to something likeplatformFilter
to disambiguate from this new setting, but I'm open to suggestions.(cc: @raptorxcz, @yonaskolb, @jlott1)