Added CARGO_BUILD_DIR_LAYOUT_V2 env var to opt in/out of new layout
#16336
Conversation
rustbot
added
A-build-execution
ranger-ross
force-pushed
the
ff-build-dir-layout
branch
from
072d232 to
b44128f
Compare
|
As an update on this, I realized I let my excitement get ahead of my analysis. We are effectively stabilizing this with this change and will need an FCP because of the community impact for making changes. In the ideal world, people would be using this env variable to start preparing their tools and if we change it, that can have a negative impact. In preparing for the FCP, I'm going through the unresolved items and future possibilities. I found a case where we aren't using the right hash and am working on fixing it. |
| Enables the new build-dir filesystem layout. | ||
| This layout change unblocks work towards caching and locking improvements. | ||
|
|
||
| In addition to `-Zbuild-dir-new-layout`, `CARGO_BUILD_DIR_LAYOUT_V2` also exists as a way to opt in and out of the new layout during the transition period. |
There was a problem hiding this comment.
Probably follow https://sembr.org/ here?
(I personally don't do line wrap in editor, so hard for me to read 😆)
There was a problem hiding this comment.
This PR adds a CARGO_BUILD_DIR_LAYOUT_V2 flag read directly from std::env::var that allows users to opt into the new layout, even on stable rust.
And eventually have an option to opt out (temporarily) once the new layout is stabilized to ease the transition for users.
Could we describe the transition plan in the PR description? Or at least briefly summarize it and provide links to any other references.
It is not clear to me what the target audience CARGO_BUILD_DIR_LAYOUT_V2 is for , and if it breaks people what we should do.
Unlike other envs like __CARGO_GITOXIDE_DISABLE_LIST_FILES whose code path block everything completely if bugs happened, CARGO_BUILD_DIR_LAYOUT_V2 seems like an opt-in only environment variable. People can enable unstable Cargo flags on stable actually today with some trick already. Without knowing the plan it seems a bit weird to add a new one.
There was a problem hiding this comment.
I plan to FCP (#16336 (comment)) and that will include the transition plan
There was a problem hiding this comment.
I wrote up my proposal for a plan at #15010 (comment)
rustbot
removed
the
S-waiting-on-review
| /// Returns true of the new build dir layout is enabled. | ||
| #[allow(clippy::disallowed_methods)] | ||
| pub fn is_new_build_dir_layout_enabled(gctx: &GlobalContext) -> bool { | ||
| match std::env::var("CARGO_BUILD_DIR_LAYOUT_V2").as_deref() { |
There was a problem hiding this comment.
Cases like #16348 have me wondering what bugs are that we don't see due to our isolated testing.
Maybe once the following are merged, do a test run of Cargo's tests with CARGO_BUILD_DIR_LAYOUT_V2=true to see what they uncover
| /// Returns true of the new build dir layout is enabled. | ||
| #[allow(clippy::disallowed_methods)] | ||
| pub fn is_new_build_dir_layout_enabled(gctx: &GlobalContext) -> bool { | ||
| match std::env::var("CARGO_BUILD_DIR_LAYOUT_V2").as_deref() { |
There was a problem hiding this comment.
Once https://github.com/rust-lang/cargo/pull/16336/files#r2593979150 is done, we'll want to do a crater run just in case
4 participants
What does this PR try to resolve?
This PR adds a
CARGO_BUILD_DIR_LAYOUT_V2flag read directly fromstd::env::varthat allows users to opt into the new layout, even on stable rust.And eventually have an option to opt out (temporarily) once the new layout is stabilized to ease the transition for users.
Note: I went with
v2incase we to do change the layout multiple times.Part of #15010
How to test and review this PR?
I added a new test that verify the flag work. We already have tests for the scenarios where the env var is not present
r? @epage