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.
[build] Allow cross-compiling build-script products for non-Darwin hosts too #36917
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
[build] Allow cross-compiling build-script products for non-Darwin hosts too #36917
Changes from all commits
ec43ef1
e9eb3b0
File 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.
@buttaface can you add a comment here (or fix up the comment above) explaining that we support it for non-darwin platforms. I am fine with a follow on commit fixing the comment.
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.
Will do.
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 simply removed the comments above, since it is no longer true. Darwin platforms were already cross-compiling in a different way, and this change adds support for non-Darwin platforms.
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.
This isn't actually going to work for cross-compiling for non-Darwin so I looked into using
install_toolchain_path
, like I did with indexstoredb below, but the concatenation directly below in this method appears to be slightly different than what that method does. Since I don't want to break the Darwin config and cross-compiling these benchmarks for non-Darwin would require other changes anyway, this should be fine for now.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 am fine with this if the tests pass (which it looks like they did).
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.
Shouldn't this also be guarded by the not is_darwin_host since you are trying to not effect the Darwin path?
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.
While most of these changes only affect non-Darwin and are guarded by such checks, there are a few changes that are applied generally, ie for Darwin too, which I called out separately in my notes for the first commit and the second commit:
Also, add a native_toolchain_path() method, that uses a prebuilt toolchain if available, and pass an install prefix in to swift-driver and sourcekit-lsp.
This was needed because non-Darwin uses a different install prefix for cross-compilation hosts, so I submitted pulls for those products' build scripts to accept an install prefix or actually start using the prefix passed in, which this pull now passes in for all platforms.
These configuration changes that affect Darwin too appear to be working fine.
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 could only replace the Darwin line above with
get_host_install_destdir()
and not this line, because this line has to point at the separate host toolchain for non-Darwin, whereas Darwin places all targets together?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.
Darwin places all targets together and then
lipo
s them into universal binaries.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.
Same as my earlier question about --prefix. I just want to understand what the effect is.
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.
Same answer as indexstoredb above.
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.
No need for a loop here if we change the SPM bootstrap flag to
--cross-compile-host
.