Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update SE-0387: Cross-Compilation Destination Bundles #1942
Update SE-0387: Cross-Compilation Destination Bundles #1942
Changes from 1 commit
28ecd8fe96b9179b560791f25bc00b525953d2a1c2a9a77c1f1851cfbe0b74bcf2deb65f39655e8cd5455d8bec2d1f98e7c7f5c7c58b2c5a515aac36ce9bc1139e51f775b427db299df2d34fdaFile filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Now that you're removing the host variant directories, have you thought about how to have these bundles use different tools based on the host, particularly on linux? Would be good to specify that, whether you plan to have different toolsets based on the build-time triple or place them in certain directories by convention.
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.
I removed host variant directories to follow what the artifact bundles proposal does. It mentions them, but ended up not using them in the example code, and I'd like to follow it as closely as possible in its implementation and bundle layout examples.
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.
OK, I took a look at that SE-0305 proposal, that I was not familiar with. It specifically mentions listing out paths to different tools based on the build-time triple needed, which is missing from this proposal. That creates an ambiguity after this pull, where you don't know which build-time triple each of these toolsets is built for, that wasn't there with the host variant directories before, with each presumably containing tools that run on that build host.
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.
Yes, it does that, but then check out example code for SwiftPM command plugins that utilize artifact bundles. Host variant directories are not present there and looks like it's convention-based after all.
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.
Sorry, not command plugins, but extensible build tools. I'm referring to this one https://github.com/abertelrud/swiftpm-buildtool-plugin-examples/tree/main/Example%201%20-%20SwiftGen/Binaries/SwiftGenBinary.artifactbundle
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.
IMO that's over-engineering toolsets for what they weren't designed for (a SwiftPM's answer to CMake toolchains, and those don't support multiple triples in a single toolchain file either). If you need different tools for a different build-time or run-time triple and Universal binaries aren't available, just create a separate toolset.
I don't think there's anything that prevents using toolsets without bundles in their current state.
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.
As you said above, that won't be enough for bundles, a whole new destination is needed.
I said "those toolsets," meaning a toolset config that works on multiple types of hosts without bundles, whereas this approach you're suggesting will require separate buildtime-specific toolset configs.
I think this is worth consolidating in the toolset spec now, as the location of the build tools for different build-time triples is a single line that shouldn't require a whole new destination that is identical except for that, but up to you.
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.
Ok, I think I see what you mean. Would a toolset at the level of artifacts reusable across multiple variants specified in
info.jsonhelp this in case? I initially had it in one of the previous commits, but felt that it reached too many levels to keep in mind when maintaining toolsets for such destinations.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.
Unsure what you're proposing, but my suggestion is to add an optional dictionary to the toolset spec, that maps each of the
supportedTriplesto their own subdirectory oftoolsetRootPath. That way SPM knows where all the build tools are and a single destination installed on linux/Windows can support multiple build-time triples, just like on Darwin.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.
Have you thought about how you want to handle this?