-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[build-script] Add option --infer to infer dependencies. #32256
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-script] Add option --infer to infer dependencies. #32256
Conversation
@swift-ci python lint |
1f965d5
to
76ab50f
Compare
@swift-ci python lint |
@swift-ci smoke test |
In a subsequent commit, I am going to make this just imply install_all since I think that is the typical behavior people will want. |
I did a little more work here and I realized I want to do install-all as part of this. Really --infer should always imply --install-all. So once I land a few other things, I am going to push the combined patch to here. Then I should be done. |
76ab50f
to
20318f6
Compare
@swift-ci python lint |
1 similar comment
@swift-ci python lint |
@swift-ci smoke test |
1 similar comment
@swift-ci smoke 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.
Nice, LGTM
This causes build-script to use the conservative dependency information that I committed. When one uses this option, it is assumed that one wants to also install all built products. Some notes: 1. I included an extra --install-all option so without --infer enabled one can enable this sort of install everything that we want to build behavior. 2. I added %cmake as a lit variable. I did this so I could specify in my build-system unit tests that on Linux they should use the just built cmake (if we built cmake due to an old cmake being on the system). Otherwise, the build system unit tests took way too long. These are meant to be dry-runs, so building this cmake again is just wasteful and doesn't make sense. 3. I unified how we handle cmark, llvm, swift with the rest of the build products by making them conditional on build_* variables, but to preserve current behavior I made it so that they are just enabled by default unlike things like llbuild/swiftpm/foundation/etc. This was necessary since previously we would just pass these flags to build-script-impl and build-script didn't know about them. Now I taught build-script about them so I can manipulate these skip-build-{cmark,llvm,swift} and then just pass them down to build-script-impl if appropriate rather than relying on build-script-impl to control if these are built. Once this lands, I think we are at a good enough place with build-script until we get rid of build-script-impl in terms of high value QoI that will imnprove productivity. Once build-script-impl is destroyed, we can start paring back what build-script itself does.
20318f6
to
a313f62
Compare
@swift-ci python lint |
I figured out the problem on Linux. We were trying to rebuild cmake. I fixed the code so we now use the just built one and also fixed the cmake check. We were comparing > instead of >= so even when I passed in the path to the just built cmake, we still tried to rebuild it since the version was the same = /. I added a >= and the problem went away. |
@swift-ci python lint |
5 similar comments
@swift-ci python lint |
@swift-ci python lint |
@swift-ci python lint |
@swift-ci python lint |
@swift-ci python lint |
@swift-ci smoke 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.
Having --infer
imply --install-all
will change the behaviour of the minimal swift build, which today does not need any installation. That may be fine, but it's not really connected to the goals of --infer
. Seems worth calling out. Anyway, LGTM.
@benlangmuir it just really simplifies the model so I don't have to specify what needs to be installed or not. We could teach build-script about that if we want to. I am trying to do minimum viable product work here to get enough of these wins to tide us over until we can simplify more of this. |
This causes build-script to use the conservative dependency information that I
committed. Keep in mind for this to work with toolchain dependent
build-products, we need for my install-all patch to land. Once that lands
though, we have a complete solution.