Skip to content
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

x.py in incremental mode still rebuilds stage0-rustc if stage0-std changed. #54712

Closed
eddyb opened this issue Oct 1, 2018 · 13 comments · Fixed by #63470
Closed

x.py in incremental mode still rebuilds stage0-rustc if stage0-std changed. #54712

eddyb opened this issue Oct 1, 2018 · 13 comments · Fixed by #63470
Labels
C-bug Category: This is a bug. P-medium Medium priority T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.

Comments

@eddyb
Copy link
Member

eddyb commented Oct 1, 2018

That is, stage0-rustc appears to lose incremental artifacts.
cc @alexcrichton @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

Yes, I believe this is because rustc/cargo will otherwise error on "found a new std" or something like that? I don't quite see how incremental is different from the normal compilation here?

@eddyb
Copy link
Member Author

eddyb commented Nov 2, 2018

@Mark-Simulacrum incremental would still work even if dependencies changed.
I think the fundamental problem here is that x.py deletes everything, whereas cargo clean (which, IIRC, does not remove incremental artifacts by default) would be appropriate.

@Mark-Simulacrum
Copy link
Member

Hm, I thought cargo clean was equivalent to rm -rf target essentially -- @alexcrichton could you clarify that? Would it be better to use cargo clean in rustbuild?

@alexcrichton
Copy link
Member

Regardless of what cargo clean does I don't think we should execute it here. Rustbuild is overly conservative in what it deletes, and it can get away probably with just deleting the deps folder instead of the entire folder. That would leave around the incremental cache.

I believe, however, that cargo clean is like rm -rf target

@eddyb
Copy link
Member Author

eddyb commented Nov 3, 2018

Maybe the cargo clean benchmarks I remember @nikomatsakis having for incremental had the incremental dir elsewhere than target.

I sure was confused by it, since I'd expect cargo clean to remove all artifacts.
If it does do that, then I agree x.py should just remove a subset of the contents of target dirs.

@eddyb
Copy link
Member Author

eddyb commented Dec 19, 2018

Looking at the code, these clear_if_dirty calls appear to be relevant:

match mode {
Mode::Std => {
self.clear_if_dirty(&my_out, &self.rustc(compiler));
},
Mode::Test => {
self.clear_if_dirty(&my_out, &libstd_stamp);
},
Mode::Rustc => {
self.clear_if_dirty(&my_out, &self.rustc(compiler));
self.clear_if_dirty(&my_out, &libstd_stamp);
self.clear_if_dirty(&my_out, &libtest_stamp);
},
Mode::Codegen => {
self.clear_if_dirty(&my_out, &librustc_stamp);
},
Mode::ToolBootstrap => { },
Mode::ToolStd => {
self.clear_if_dirty(&my_out, &libstd_stamp);
},
Mode::ToolTest => {
self.clear_if_dirty(&my_out, &libstd_stamp);
self.clear_if_dirty(&my_out, &libtest_stamp);
},
Mode::ToolRustc => {
self.clear_if_dirty(&my_out, &libstd_stamp);
self.clear_if_dirty(&my_out, &libtest_stamp);
self.clear_if_dirty(&my_out, &librustc_stamp);
},
}

The problem is that the directory my_out refers to is non-trivially structured.

Instead of removing $my_out/*, we need to remove all the files (or at least the ones that are artifacts) and the deps directory in $my_out{/$target,}/{release,debug}/.

@jonas-schievink jonas-schievink added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) C-bug Category: This is a bug. labels Jan 27, 2019
@eddyb
Copy link
Member Author

eddyb commented Apr 23, 2019

I keep hitting this, specifically it keeps rebuilding rustc_codegen_llvm and rustdoc, every time I change rustc, which cause them to take much longer than any of the rustc crates, and this issue has become my main bottleneck (now that I'm building on a -j48 machine).

@eddyb eddyb added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Apr 23, 2019
@eddyb
Copy link
Member Author

eddyb commented Apr 23, 2019

Nominating for discussion (specifically, allocating resources towards this) at the next relevant team meeting (tagged both @rust-lang/infra and @rust-lang/compiler, as I'm not sure which has "jurisdiction" here).

@Mark-Simulacrum
Copy link
Member

It'd be helpful to know how much rustbuild can "trust" the compiler here. Can we keep incremental artifact directories around from previous versions of the compiler?

@michaelwoerister
Copy link
Member

The compiler can only work with incremental artifacts that it created itself. For any other version of the compiler it has to assume that it's incompatible. It usually just ignores such incompatible artifacts, but in the bootstrapping scenario, it might not always be smart enough to do that.

@Zoxc
Copy link
Contributor

Zoxc commented Apr 25, 2019

I think the question here is will rustc's incremental artifacts correctly deal with crates changing (including adding and removing crates) in the sysroot. If so, we could keep the incremental folder around. Cargo assumes the sysroot is unchanging, which is why we remove the target directory in the first place.

@eddyb
Copy link
Member Author

eddyb commented Apr 30, 2019

Incremental doesn't care about --extern vs -L, so I'd expect it to be pretty resilient here.

@pnkfelix
Copy link
Member

pnkfelix commented May 2, 2019

triage: P-medium. (this inefficiency in incremental comp. would be nice to resolve, but is not P-high IMO.)

@pnkfelix pnkfelix added the P-medium Medium priority label May 2, 2019
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Aug 14, 2019
…xcrichton

Utilize -Zbinary-dep-depinfo in rustbuild

The last commit moves us over to using binary-dep-depinfo while the first two permit us to bootstrap from what will become future beta, to be released in the next week (it's the `cfg(bootstrap)` processing).

We no longer utilize stamp-file mtimes at all inside rustbuild, and a future PR may be able to entirely eliminate them by eagerly copying to the appropriate sysroot. The only mtime-based dependency tracking left is for documentation because we lie to Cargo about the rustdoc binary, so Cargo does not track changes to the real binary, and codegen-backends because binary-dep-depinfo does not emit that information into the depfiles.

Both of these are fixable in the longer term but this existing patch gives us the following benefits:
 * We no longer delete Cargo target directories manually within a stage. Cross-stage, changes to codegen backends will still clear out target directories. This means that incremental state persists across individual steps (e.g., rebuilding libstd does not clear out librustc incremental state). Fixes rust-lang#54712.
 * Dependency tracking across steps within a given stage is now fully precise. We will not clear out all codegen backend dependencies due to changes in librustc_driver, for example, only deleting the final librustc_codegen_llvm crate. Fixes rust-lang#54008, fixes rust-lang#50481.
 * We properly track codegen backends as a dependency (equivalent to rustc) across changes. Fixes rust-lang#53284, and fixes rust-lang#52719.
 * Cross-stage dependency tracking of crates is also much more accurate and reliable. Most likely fixes rust-lang#49979 (but no reproduction steps in that issue). Fixes rust-lang#59105.

cc rust-lang#63012
Centril added a commit to Centril/rust that referenced this issue Aug 15, 2019
…xcrichton

Utilize -Zbinary-dep-depinfo in rustbuild

The last commit moves us over to using binary-dep-depinfo while the first two permit us to bootstrap from what will become future beta, to be released in the next week (it's the `cfg(bootstrap)` processing).

We no longer utilize stamp-file mtimes at all inside rustbuild, and a future PR may be able to entirely eliminate them by eagerly copying to the appropriate sysroot. The only mtime-based dependency tracking left is for documentation because we lie to Cargo about the rustdoc binary, so Cargo does not track changes to the real binary, and codegen-backends because binary-dep-depinfo does not emit that information into the depfiles.

Both of these are fixable in the longer term but this existing patch gives us the following benefits:
 * We no longer delete Cargo target directories manually within a stage. Cross-stage, changes to codegen backends will still clear out target directories. This means that incremental state persists across individual steps (e.g., rebuilding libstd does not clear out librustc incremental state). Fixes rust-lang#54712.
 * Dependency tracking across steps within a given stage is now fully precise. We will not clear out all codegen backend dependencies due to changes in librustc_driver, for example, only deleting the final librustc_codegen_llvm crate. Fixes rust-lang#54008, fixes rust-lang#50481.
 * We properly track codegen backends as a dependency (equivalent to rustc) across changes. Fixes rust-lang#53284, and fixes rust-lang#52719.
 * Cross-stage dependency tracking of crates is also much more accurate and reliable. Most likely fixes rust-lang#49979 (but no reproduction steps in that issue). Fixes rust-lang#59105.

cc rust-lang#63012
Centril added a commit to Centril/rust that referenced this issue Aug 15, 2019
…xcrichton

Utilize -Zbinary-dep-depinfo in rustbuild

The last commit moves us over to using binary-dep-depinfo while the first two permit us to bootstrap from what will become future beta, to be released in the next week (it's the `cfg(bootstrap)` processing).

We no longer utilize stamp-file mtimes at all inside rustbuild, and a future PR may be able to entirely eliminate them by eagerly copying to the appropriate sysroot. The only mtime-based dependency tracking left is for documentation because we lie to Cargo about the rustdoc binary, so Cargo does not track changes to the real binary, and codegen-backends because binary-dep-depinfo does not emit that information into the depfiles.

Both of these are fixable in the longer term but this existing patch gives us the following benefits:
 * We no longer delete Cargo target directories manually within a stage. Cross-stage, changes to codegen backends will still clear out target directories. This means that incremental state persists across individual steps (e.g., rebuilding libstd does not clear out librustc incremental state). Fixes rust-lang#54712.
 * Dependency tracking across steps within a given stage is now fully precise. We will not clear out all codegen backend dependencies due to changes in librustc_driver, for example, only deleting the final librustc_codegen_llvm crate. Fixes rust-lang#54008, fixes rust-lang#50481.
 * We properly track codegen backends as a dependency (equivalent to rustc) across changes. Fixes rust-lang#53284, and fixes rust-lang#52719.
 * Cross-stage dependency tracking of crates is also much more accurate and reliable. Most likely fixes rust-lang#49979 (but no reproduction steps in that issue). Fixes rust-lang#59105.

cc rust-lang#63012
Centril added a commit to Centril/rust that referenced this issue Aug 15, 2019
…xcrichton

Utilize -Zbinary-dep-depinfo in rustbuild

The last commit moves us over to using binary-dep-depinfo while the first two permit us to bootstrap from what will become future beta, to be released in the next week (it's the `cfg(bootstrap)` processing).

We no longer utilize stamp-file mtimes at all inside rustbuild, and a future PR may be able to entirely eliminate them by eagerly copying to the appropriate sysroot. The only mtime-based dependency tracking left is for documentation because we lie to Cargo about the rustdoc binary, so Cargo does not track changes to the real binary, and codegen-backends because binary-dep-depinfo does not emit that information into the depfiles.

Both of these are fixable in the longer term but this existing patch gives us the following benefits:
 * We no longer delete Cargo target directories manually within a stage. Cross-stage, changes to codegen backends will still clear out target directories. This means that incremental state persists across individual steps (e.g., rebuilding libstd does not clear out librustc incremental state). Fixes rust-lang#54712.
 * Dependency tracking across steps within a given stage is now fully precise. We will not clear out all codegen backend dependencies due to changes in librustc_driver, for example, only deleting the final librustc_codegen_llvm crate. Fixes rust-lang#54008, fixes rust-lang#50481.
 * We properly track codegen backends as a dependency (equivalent to rustc) across changes. Fixes rust-lang#53284, and fixes rust-lang#52719.
 * Cross-stage dependency tracking of crates is also much more accurate and reliable. Most likely fixes rust-lang#49979 (but no reproduction steps in that issue). Fixes rust-lang#59105.

cc rust-lang#63012
Centril added a commit to Centril/rust that referenced this issue Aug 15, 2019
…xcrichton

Utilize -Zbinary-dep-depinfo in rustbuild

The last commit moves us over to using binary-dep-depinfo while the first two permit us to bootstrap from what will become future beta, to be released in the next week (it's the `cfg(bootstrap)` processing).

We no longer utilize stamp-file mtimes at all inside rustbuild, and a future PR may be able to entirely eliminate them by eagerly copying to the appropriate sysroot. The only mtime-based dependency tracking left is for documentation because we lie to Cargo about the rustdoc binary, so Cargo does not track changes to the real binary, and codegen-backends because binary-dep-depinfo does not emit that information into the depfiles.

Both of these are fixable in the longer term but this existing patch gives us the following benefits:
 * We no longer delete Cargo target directories manually within a stage. Cross-stage, changes to codegen backends will still clear out target directories. This means that incremental state persists across individual steps (e.g., rebuilding libstd does not clear out librustc incremental state). Fixes rust-lang#54712.
 * Dependency tracking across steps within a given stage is now fully precise. We will not clear out all codegen backend dependencies due to changes in librustc_driver, for example, only deleting the final librustc_codegen_llvm crate. Fixes rust-lang#54008, fixes rust-lang#50481.
 * We properly track codegen backends as a dependency (equivalent to rustc) across changes. Fixes rust-lang#53284, and fixes rust-lang#52719.
 * Cross-stage dependency tracking of crates is also much more accurate and reliable. Most likely fixes rust-lang#49979 (but no reproduction steps in that issue). Fixes rust-lang#59105.

cc rust-lang#63012
Centril added a commit to Centril/rust that referenced this issue Aug 15, 2019
…xcrichton

Utilize -Zbinary-dep-depinfo in rustbuild

The last commit moves us over to using binary-dep-depinfo while the first two permit us to bootstrap from what will become future beta, to be released in the next week (it's the `cfg(bootstrap)` processing).

We no longer utilize stamp-file mtimes at all inside rustbuild, and a future PR may be able to entirely eliminate them by eagerly copying to the appropriate sysroot. The only mtime-based dependency tracking left is for documentation because we lie to Cargo about the rustdoc binary, so Cargo does not track changes to the real binary, and codegen-backends because binary-dep-depinfo does not emit that information into the depfiles.

Both of these are fixable in the longer term but this existing patch gives us the following benefits:
 * We no longer delete Cargo target directories manually within a stage. Cross-stage, changes to codegen backends will still clear out target directories. This means that incremental state persists across individual steps (e.g., rebuilding libstd does not clear out librustc incremental state). Fixes rust-lang#54712.
 * Dependency tracking across steps within a given stage is now fully precise. We will not clear out all codegen backend dependencies due to changes in librustc_driver, for example, only deleting the final librustc_codegen_llvm crate. Fixes rust-lang#54008, fixes rust-lang#50481.
 * We properly track codegen backends as a dependency (equivalent to rustc) across changes. Fixes rust-lang#53284, and fixes rust-lang#52719.
 * Cross-stage dependency tracking of crates is also much more accurate and reliable. Most likely fixes rust-lang#49979 (but no reproduction steps in that issue). Fixes rust-lang#59105.

cc rust-lang#63012
@bors bors closed this as completed in 9a32ad0 Aug 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-medium Medium priority T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants