-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add a --compile-time-deps
build flag to cargo build
#3344
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
Conversation
cc @rust-lang/wg-rls-2 @Undin @vlad20012 |
Which of the following spellings should be preferred: |
|
To build the tests today, the appropriate command is In practice though, |
This will also be useful when builder Docker images - |
For Docker caching you did want to build all dependencies outside of the workspace, right? Not just build dependencies, proc macros and run build scripts for any package including those in the workspace as this RFC proposes AFAICT. |
For docker, see rust-lang/cargo#2644, particularly the summary, for why this or most solutions will not work. |
|
||
The current alternatives are the `RUSTC_WRAPPER` or a plain `cargo check` invocation as has been outlined in the motivation section. | ||
- The `RUSTC_WRAPPER` mostly works today, but this is not necessarily its intended usage, and it is unknown what other problems such a wrapper unconditionally succeeding invoked commands could cause next. | ||
- The plain `cargo check` is not a decent option for IDEs, as the IDE loses the ability to discern between build-script problems and workspace diagnostics. |
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.
Do the messages, combined with knowing what a workspace member is, convey enough information that these can instead be filtered?
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.
Whether something is a workspace member or not is irrelevant, we'd need to be able to know whether a diagnostic comes from building a build-script / proc-macro crate. We'd also still need to rely on the unstable keep-going flag. Turns out that one is stable now, nice.
So assuming we can do that filtering, that would be an option, though this would still be slower than the current wrapper approach / a future way to only build / run the build dependencies.
|
||
- Should this be a flag for `cargo build`, or does it require its own subcommand? | ||
- Compilation options do not affect build scripts or proc-macros in `cargo build` invocations, likewise the proposal for the flag here therefor defines the same. Is there a use case where it would make sense to allow compilation options to affect these artifacts when the proposed flag is supplied? | ||
- Does the flag name fit or is there a better name for this? |
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.
The two concepts we talk to users about build scripts and proc macros are
- build dependencies
- building for the host
# Reference-level explanation | ||
[reference-level-explanation]: #reference-level-explanation | ||
|
||
Add a flag `--compile-time-deps` to `cargo build` that will cause cargo to only build proc-macros and build-scripts as well as running the build-scripts. |
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 take it this means that it would exclusively build proc macro direct dependencies and build scripts for root crates (user-selected crates, implicitly or explicitly), and all of the dependencies necessary for doing that?
Would be good to be more explicit
[rationale-and-alternatives]: #rationale-and-alternatives | ||
|
||
The current alternatives are the `RUSTC_WRAPPER` or a plain `cargo check` invocation as has been outlined in the motivation section. | ||
- The `RUSTC_WRAPPER` mostly works today, but this is not necessarily its intended usage, and it is unknown what other problems such a wrapper unconditionally succeeding invoked commands could cause next. |
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.
Here's what it causes next: build cache corruption.
rust-lang/cargo#13485
Aside from building and running these things, this is also needed for the IDE to figure out things like a package's `OUT_DIR` location, or the file path of a build proc-macro which is read from cargo's metadata output. | ||
|
||
IDEs can already do this with `cargo` as is, by just running `cargo check` as this builds proc-macros and *runs* build scripts already, but this also checks the rest of the project which for this step depending on the project in question may be quite costly using up time and resources for something the IDE is not even interested in at this point. | ||
This also has the additional downside that this pollutes the metadata output with compiler diagnostics of the project, so if the project fails to build, but the proc-macros and build scripts work fine, the IDE now sees an error in this step where none should be, possibly unnecessarily poking the user about this step failing even though it technically didn't. |
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.
If you are using the json output, couldn't you determine that the error is not relevant and ignore it?
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.
Kind of? We'd need to walk the reverse dependency tree for each diagnostic to check if its part of a build dependency to ignore it. I think. As we do want to report if the build script fails to compile (and hence run)
We discussed this in today's cargo meeting Would r-a be able to use this as a never-to-stabilize option? It feels like there is another feature hinted at by this need and it would help to accumulate use cases over time in dealing with package / built-target selection to better understand how to serve that need. We don't want to block the improvement of IDEs until that happens though. We are sympathetic to the use case and would like to see a solution. This has the benefit of not boxing us into a corner with the build graph because of how specialized it is. However, that specialization is its own hurdle when it comes to the user experience.
|
Yes, we already use a number of unstable features (i.e. |
In that case and if @Veykril, as the RFC author, is on board, then we can proceed with this as an issue in the Cargo repo, rather than an RFC. Assumes its marked as "accepted" (I'll mark it as such as soon as someone creates it). |
Important thing is that there should be a way to use this unstable feature on a stable toolchain without RUSTC_BOOTSTRAP=1. Another environment variable is an option |
That's not how unstable features work? |
I guess one concern is how it might interact with feature detection done by build scripts. An unstable Cargo feature might be better anyway. |
For that rust-analyzer could use |
Won't that cause rebuilds? Also that itself is a
That is fine by me, that will at least make things better on nightly toolchains. I wasn't expecting this RFC to become stable that quickly either way (there is a lot of use cases to explore still as you mentioned). Using it on non-nightly toolchains is a good question though since we can't use I guess we could repurpose the |
Right, it does.
If you set |
Shouldn't this be a Cargo feature anyway? Those don't need |
Cargo checks |
Opened rust-lang/cargo#14434 |
Some build scripts emit cargo::rerun-if-changed=RUSTC_BOOTSTRAP. Namely, this does If rust-analyzer invokes cargo with RUST_BOOTSTRAP=1 in a project with such a build script, the project will be rebuilt almost from scratch during the next cargo build invocation without RUSTC_BOOTSTRAP=1. Basically, a project would be rebuilt from scratch after opening in rust-analyzer. This is why requiring RUSTC_BOOTSTRAP=1 for this feature is very unsatisfactory. May we have another hacky variable specially for this use case? |
Looks like we might already: rust-lang/cargo#14434 (comment) I will close this RFC then in favor of that cargo issue as the place for discussion |
Rendered