-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[build-script] Introduce ProductBuilder. Transform Ninja to use it. #23822
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
Merged
drodriguez
merged 1 commit into
swiftlang:master
from
drodriguez:build-script-product-builder-ninja
May 14, 2019
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
None of this is your doing, I'm wondering if you know the history. Why is this in
StdlibDeploymentTarget
? Why is this being built for thehost_target
and notbuild_target
? This is the tool being built for the build right?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 think
StdlibDeploymentTarget
is a little misnamed at this point, because it is used for more things that just the stdlib. If you think about it asCompilationTarget
orDeploymentTarget
is easier to understand.Ninja is build only for the machine building the rest of the pieces (it is build if it cannot be found in the toolchain/path), so it is fine to “hardcode” here to
host_target
(which is the current machine).If you look at the implementation of
make_builder
, it actually ignores the target. The previous implementation didn’t care about the target (so it builds for the local target).PD: there's a big mess of names in the build script. Sometimes
host
refers to the device that run the code (arguments--host-test
and--skip-test-ios-host
which control to run or not tests on the external devices), but sometimes refers to the machine doing the build (like thishost_target
and arguments like--build-runtime-with-host-compiler
and--host-cc
).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.
Right, I figured it was a poorly named variable at this point. I was thinking of it as
TargetInformation
. I was wondering if you knew why this had come to be poorly named at this point.As to the Ninja bits, right - its again a name thing.
target
is the target where generated code will be run,host
is where the generated code generator will run, andbuild
is the machine where the build is occurring. These are well documented names, so it was a question of does the current machinery not name things accordingly?I think that making it deal with cross-compilation is great thing, but way outside the scope of this current change .
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.
About the renaming, from comments and clues in the code, I think there was a previous try to make everything more standard, but it was necessary to keep the argument names the same for compatibility, which probably makes things a little bit more complicated. I can see if something can be done as a cleanup when all these pieces fall into place (I prefer to have the “wrong” names, since they match better what the
build-script-impl
have).