-
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
Reconsider RUSTFLAGS artifact caching. #8716
Comments
My main pain point with RUSTFLAGS is xd009642/tarpaulin#416 (comment), which has some niche flags and wouldn't be helped much by this. AFAIK all of those flags are codegen/linking related though, and don't affect
Maybe there can be a way for I don't know enough about |
I believe this does because the filenames of object files show up as the codegen unit names in dwarf debug info (and probably PDB?). I suspect that everything else in the binary will be the same, but builds differing only in
I think this will work but I don't think it'll be too feasible if the default is to not hash RUSTFLAGS. The pain of rebuilding too much you only discover after-the-fact that you should have been hashing rustflags all along. I don't think we can hash-by-default though since it breaks the use cases you've mentioned.
I do think a Another possible solution could be adding a new variable like |
building static-linked runtimetest requires additional RUSTFLAGS env var. Unfortunately, the additional env var invalids the build cache, leading cargo rebuilding everytime. See rust-lang/cargo#8716 A temporary solution is to build runtimetest in a seperate `runtimetest-target` directory. Signed-off-by: Tony Duan <tony84727@gmail.com>
building static-linked runtimetest requires additional RUSTFLAGS env var. Unfortunately, the additional env var invalids the build cache, leading cargo rebuilding everytime. See rust-lang/cargo#8716 A temporary solution is to build runtimetest in a seperate `runtimetest-target` directory. Signed-off-by: Tony Duan <tony84727@gmail.com>
For some scenarios it is better to not set Rustflags for all crates in the dependency graph and instead only set it for the top-level crate. For example rust-lang/cargo#8716 can be avoided in some scenarios by setting the rustflags via rustc, which allows for faster rebuilds in such cases.
For some scenarios it is better to not set Rustflags for all crates in the dependency graph and instead only set it for the top-level crate. For example rust-lang/cargo#8716 can be avoided in some scenarios by setting the rustflags via rustc, which allows for faster rebuilds in such cases.
For some scenarios it is better to not set Rustflags for all crates in the dependency graph and instead only set it for the top-level crate. For example rust-lang/cargo#8716 can be avoided in some scenarios by setting the rustflags via rustc, which allows for faster rebuilds in such cases.
For some scenarios it is better to not set Rustflags for all crates in the dependency graph and instead only set it for the top-level crate. For example rust-lang/cargo#8716 can be avoided in some scenarios by setting the rustflags via rustc, which allows for faster rebuilds in such cases.
For some scenarios it is better to not set Rustflags for all crates in the dependency graph and instead only set it for the top-level crate. For example rust-lang/cargo#8716 can be avoided in some scenarios by setting the rustflags via rustc, which allows for faster rebuilds in such cases.
We've used a similar pattern in past projects. One thing that has changed is that we use `[target.'cfg(feature = "cargo-clippy")']` instead of `[build]`. This allows us to enforce this lint configuration locally and not just on CI. Using `rustflags` does mean that devs might encounter unexpected full rebuilds when switching between IDE and command line when using `cargo clippy`. This is a known issue[1]. For emacs, I just solved this by setting a different Cargo `target-directory` to the one used by default. A similar approach should work for other IDEs. [1]: rust-lang/cargo#8716.
We've used a similar pattern in past projects. One thing that has changed is that we use `[target.'cfg(feature = "cargo-clippy")']` instead of `[build]`. This allows us to enforce this lint configuration locally and not just on CI. Using `rustflags` does mean that devs might encounter unexpected full rebuilds when switching between IDE and command line when using `cargo clippy`. This is a known issue[1]. For emacs, I just solved this by setting a different Cargo `target-directory` to the one used by default. A similar approach should work for other IDEs. [1]: rust-lang/cargo#8716.
235: Add Clippy lint workspace configuration r=luckysori a=luckysori We've used a similar pattern in past projects. One thing that has changed is that we use `[target.'cfg(feature = "cargo-clippy")']` instead of `[build]`. This allows us to enforce this lint configuration locally and not just on CI. Using `rustflags` does mean that devs might encounter unexpected full rebuilds when switching between IDE and command line when using `cargo clippy`. This is a known [issue]. For emacs, I just solved this by setting a different Cargo `target-directory` to the one used by default. A similar approach should work for other IDEs. [issue]: rust-lang/cargo#8716. Co-authored-by: Lucas Soriano del Pino <lucas_soriano@fastmail.com>
We've used a similar pattern in past projects. One thing that has changed is that we use `[target.'cfg(feature = "cargo-clippy")']` instead of `[build]`. This allows us to enforce this lint configuration locally and not just on CI. Using `rustflags` does mean that devs might encounter unexpected full rebuilds when switching between IDE and command line when using `cargo clippy`. This is a known issue[1]. For emacs, I just solved this by setting a different Cargo `target-directory` to the one used by default. A similar approach should work for other IDEs. [1]: rust-lang/cargo#8716.
235: Add Clippy lint workspace configuration r=luckysori a=luckysori We've used a similar pattern in past projects. One thing that has changed is that we use `[target.'cfg(feature = "cargo-clippy")']` instead of `[build]`. This allows us to enforce this lint configuration locally and not just on CI. Using `rustflags` does mean that devs might encounter unexpected full rebuilds when switching between IDE and command line when using `cargo clippy`. This is a known [issue]. For emacs, I just solved this by setting a different Cargo `target-directory` to the one used by default. A similar approach should work for other IDEs. [issue]: rust-lang/cargo#8716. Co-authored-by: Lucas Soriano del Pino <lucas_soriano@fastmail.com>
Another option is for cargo to inspect RUSTFLAGS and avoid rebuilding for flags it knows can only affect leaf crates or diagnostics, like -C link-args or -W lint flags. I know cargo has a policy of not inspecting RUSTFLAGS but I don't know the history or rationale for that decision. |
A note from a user: this issue also appears when one wants to build multiple target flavors of WASM, for example to have two versions, one basic and one with |
I agree that something like this should be done. As rust-lang/rust#110654 points out, current behaviour can be annoying. First of all, one of the main focuses on the PR — changes in rustflags should not invalidate the cache. Bullet point 1 seems like the only way to solve it.
As pointed out:
There seems to be no road forward without refactoring something or adding something new - but what? For over 99% of users (it seems like just those who need reproducible builds AND care about the filenames of the units) the solution is one small tweak away. Can we expect the user to know if they care about filenames? Maybe this flag can propagate from the dependencies to the leaves? What if units were simlinked (or hardlinked) to something not including the extra-filename? The links can be overwriten at neglible cost, while debuggers get a constant filename to work with. Maybe I'm missing the point of Secondly, not all RUSTFLAGS values need to be fingerprinted.
The great thing about this is that building a hashing blacklist is incremental and non-breaking. Even if some values are forgotten, it would still be better than now. It does not solve the problem for when something rebuilds, but it does make rebuilds happen less often. I see no downsides. |
CLI parsing is complex and having cargo parse another program's CLI means cargo needs to duplicate the knowledge of that other programs CLI. Trying to parse a subset of known flags has its own caveats that for when it falls down flat. Compared to rustup, we at least have the advantage that these CLIs ship together. |
Another angle to this is that for #5931, I'd like us to avoid dealing with UX problems of the cache being poisoned, so I'd favor accidentally including too much in the fingerprint than not enough. |
Does this mean that currently changing the order of rustflags, or changing |
From my reading of the code, yes, superficial changes like that will cause rebuilds. One of the potential solutions in the original issue was to provide first-class support for some of the features. #12115 is an example of doing that and came from the pain I had from trying to use RUSTFLAGS for lint levels. Granted, that protects against order changes and |
Given that the current semantics of But my question is, isn't the current behavior the biggest possible hammer? Could a solution involve providing a limited-scope version of I don't know whether this scales to many of the Edit: Thinking about it more, this is architecturally orthogonal to the issue of not wanting some particular flags to affect the symbol hashes of the crate, because that's an issue for the root binary. So this idea maybe helps with reducing the "blast zone" of |
Another variant of the above idea would be We could potentially even say that ordinary I know it's another knob and we hate those, but really we need something less pervasive than |
Edit: perhaps this as well rust-lang/rfcs#3310? But if you only concern binaries not final artifacts, my point is not relevant. |
You're absolutely right, that RFC is very much related to this idea. I'm not sure which one would work better for more scenarios, but I'll consider it.
It seems that it can, but I didn't know about it and it seems little used in the community, or else why would the "PGO options in RUSTFLAGS" be such a problem? Is there some reason why Your point's well taken though, and I wonder whether |
When RUSTFLAGS is changed between invocations, this can trigger a complete rebuild. This can be annoying, and the only workaround is to use a separate target directory for each RUSTFLAGS setting. I think we should revisit how this works, and see if there is a way to address the problems.
Motivation:
History:
--remap-path-prefix
from the hash, because it broke reproducible builds. 1.37 (2019-08-15)RUSTFLAGS
in-Cmetadata
#7417 Reverted Rustflags in metadata #6503 because it broke profile guided optimization. 1.39 (2019-11-07)Solutions?
I am uncertain what a good approach would be. Some rough ideas:
-C extra-filename
, and not-C metadata
. This should (probably) avoid the PGO problems, since the symbols won't change. However, this could still cause problems with reproducible builds if the rlib filename somehow leaks into the resulting binary (I don't have a reason to think it does, but it seems risky).The text was updated successfully, but these errors were encountered: