-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat(build-plan): output invocations after running build scripts #7123
feat(build-plan): output invocations after running build scripts #7123
Conversation
Extend the `--build-plan` option with named value `STAGE` that indicates at which point in the build process to output the (remaining) invocations. The (up to now) only behavior is labeled as the `init` stage (compile nothing, add all invocations to the build plan) and a new stage is added, `post-build-scripts`, to compile and run custom build scripts (along with their dependencies) and generate the build plan with the invocations of the remaining units (that were actually going to be compiled and linked in the final binary/library). Add a new attribute to `Unit` (actually `UnitInner`), `to_be_compiled`, to signal (depending on the build plan selected, if any) which unit will be compiled and which added to the build plan (see its documentation for the full rationale of its inclusion in `Unit`). Replace the binary logic of local `build_plan: bool` variables, signaling if the `--build-plan` option is used, with the `to_be_compiled` attribute that contains a more fine-grained logic to decide which unit will be compiled (based on the build plan selected). This change is seen in the diff as (roughly) the replacement of `build_plan` (now deprecated) with the negated value (but same meaning) in `!to_be_compiled`. Add two tests for the new stage, and modify the basic test (`cargo_build_plan_simple`) to explicitly include the stage name `init` to test the new label (the rest of the build plan tests remain the same, to check backwards compatibility with the previous `--build-plan`-only use).
(rust_highfive has picked a reviewer for you, use r? to override) |
// FIXME: The JSON matching pattern in this test is very brittle, we just care | ||
// about the above *extra* items in the `"args"` and `"env"` arrays, but we need | ||
// to add *all* their elements for the pattern to match, there doesn't seem to | ||
// be a more flexible wildcard of the sort `{... "extra-item"}`. |
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.
Failing for non-Linux targets, I'll try to implement a wildcard of the sort.
☔ The latest upstream changes (presumably #7069) made this pull request unmergeable. Please resolve the merge conflicts. |
Sorry for the delay and lack of messaging here @schomatis. Unfortunately though the build-plan mechanism is pretty lackluster in Cargo and is full of a lot of bugs and doesn't have a great plan for moving forward. I'm going to close this because I don't think we're going to take major changes to the build-plan infrastructure because it needs to sort of start from scratch at this point. We're hopeful that we can find some time to set the direction for build plan in the future, but unfortunately it will require a relatively large investment on the team's part to lay out the current pros/cons and how we imagine them getting fixed. |
Without any feedback on the original issue #7068 I'm going ahead and submitting this PR to flesh out the details with the actual code, but feel free to close it if this wasn't an acceptable change in the first place.
The main change in the current architecture (and whose rationale should be reviewed in more depth than the rest of the code) is the inclusion of the new
to_be_compiled
attribute directly inUnitInner
(and not in one of its subcomponents likeCompileMode
orProfile
).Closes #7068.