-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Verify product dependencies w/ target platform, vs. hardcoded macOS
#6963
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
Verify product dependencies w/ target platform, vs. hardcoded macOS
#6963
Conversation
This was a fun one to chase down 😅 Some questions:
|
Please run
Yes, please add the tests, that would be great. Thanks! |
Thanks! I'll take a crack at it soon :) |
…latform # Conflicts: # Sources/Build/BuildPlan.swift
Cherry-picked given OG file was moved into `BuildPlan/` folder
- Tweaked existing validation test to ensure no overlap w/ this new test - Added `Triple.arm64iOS` for conveinence
Manually triggered by adding newline (which formatter would auto-remove) to help make PR diff a bit cleaner :)
@MaxDesiatov Apologies for the delay on this! One test added, one test modified 🧪 Since |
Thanks! Just as heads up, this will conflict with #7161 |
…into peterkos/validate-product-deps-against-env-platform # Conflicts: # Sources/Build/BuildPlan/BuildPlan.swift
Resolved conflicts 👍 Looks like // BuildPlanTests L#5749
func testArtifactsArchiveBinaryTargets(...) {
// ...
"path": "all-platforms/mytool",
"supportedTriples": ["\(
artifactTriples.map(\.tripleString)
.joined(separator: "\", \"")
)"]
}
]
}
}
}
"""
// ^ swiftformat throws this here,
// but is a swift build error :(
I've manually reverted the script's raw string changes for now, but it seems like the way swiftformat expands inline closures (among other things) could be improved -- that's my guess 🚀 Also the swiftformat $changed_file > dev/null 2>&1 # old
swiftformat $changed_file 2>&1 # new |
@swift-ci test |
@swift-ci test windows |
@swift-ci test |
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.
Thank you!
macOS
macOS
@swift-ci test windows |
Product dependencies are now verified using the
buildEnvironment
's platform, instead of the hardcodedmacOS
platform.This builds on the work of #6732, now allowing cross-compilation of product dependencies 🎉
Motivation:
Let's say we're cross-compiling macOS -> iOS simulator, with a host system of
macOS v13
.As long as we set our triple & SDK, we should be good... (and, with #6828, only the triple needed)
Even though
SwiftFlashlight
only supportsiOS(.v16)
, SupportedPlatform "[by] default, [...] assigns a predefined minimum deployment version for each supported platforms unless you configure supported platforms [...]". As a side-effect,macOS
is inferred as a valid platform. (This is why no "target/platform mismatch" error was thrown.)Previously, product dependencies were verified against
macOS
, rather than the target platform. SincemacOS
is always inferred, this surfaced as a "woah! your dependencies are out of sync".Now, the intention of
--triple
is respected 🐱Modifications:
validateDeploymentVersionOfProductDependency
accepts abuildEnvironment
containing the target platformResult:
Product dependencies can now be compiled for
darwin
other than macOS -- i.e, iOS :)Future work
triple.isDarwin()
-- perhaps a next step of, verifying dependencies against non-darwin?