Make sure to clear out the stageN-{rustc,std,tools} directories. #44792

Mark-Simulacrum · 2017-09-23T17:13:07Z

We copy built tool binaries into a dedicated directory to avoid deleting them,
stageN-tools-bin. These aren't ever cleared out by code, since there should be
no reason to do so, and we'll simply overwrite them as necessary.

When clearing out the stageN-{std,rustc,tools} directories, make sure to delete
both Cargo directories -- per-target and build scripts. This ensures that
changing libstd doesn't cause problems due to build scripts not being rebuilt,
even though they should be.

Fixes #44739.

alexcrichton · 2017-09-25T15:16:28Z

Unfortunately at this point I honestly forget why this is architected the way it is and how it worked at some point. I'm a little worried here about the loss of stamp files as it may cause us to probe quite a few files on builds, and I'm also a little worried about the global atomic hack needed here. Could you expand a bit on these two changes and why a stamp file didn't cut it and why the step always nees to run?

Mark-Simulacrum · 2017-09-25T17:24:39Z

I didn't really investigate why the stamp file doesn't cut it -- I can try to do that. I suspect it's because we don't actively touch it/update it properly when running Cargo, but wouldn't be too certain. It's also certainly less reliable than the method I chose (which seems to have minimal impact, at least on Linux -- I didn't notice any slowdown with my patch on noop builds).

The global atomic hack exposes a weakness of our current caching implementation, combined with how we build tools. Basically, if the sequence of the build is libstd -> tidy (std tool) -> test/rustc -> error-index-generator (librustc dependent tool), then we'll clear out the tools directory while build libtest and librustc, which means we need to recompile tools. Previously we would've errored out when trying to run tidy later, but now we will rebuild it and properly run it. This may be better solved by moving tools to stageN-tools-{std,rustc,test} or something like that, but I'm not a fan of that solution either.

We can either file a mentored issue for the CleanTools refactor (it should just be a method, though perhaps with some per-stage caching... not really necessary, shouldn't have a major effect) or I can implement that in this PR, if you'd like. We still need the global atomic or some way to clear all tools in a given stage out of the "built cache," though, since we'll otherwise not rebuild tools properly after we've cleaned them out.

alexcrichton · 2017-09-25T17:41:35Z

Ok for the stamp file I'd prefer to not probe the entire directory, so mind checking to see what's going on there?

For cleaning tools I looked into this and this is how it used to work. This previously worked because of "order only dependencies". That is, the "clean all tools" step didn't actually depend on libstd/libtest/librustc, but it always ran after those steps if they were present in the dependency graph.

I don't think that strategy is implementable today because we don't ever know the dependency graph. A worry I have about this PR as-is, though, is something like:

build libstd
clean tools dir
build tidy
build libtest
clean tools dir
build another tool

After we build libtest the cleaning of the tools dir ends up blowing away the build tidy binary when it didn't get invalidated. The only solution I can think of right now is stageN-tools-{std,rustc,test}, but you're right in that it's not a great solution...

Thinking some more...

I wonder if maybe we could do something like copy the tools out of the stageN-tools directory? That way when we blow away the directory twice the tool is still persisting and we won't rebuild it/

Mark-Simulacrum · 2017-09-26T01:46:01Z

We can copy the tools out of the tools directory -- I actually like that idea, since we could even potentially copy them into stageN/bin which would possibly allow rustup link to permit running the tools directly. What do you think about that location?

It may be worth pointing out though that with this PR as written, we don't (I hope) expect tools today to exist unless we've called ensure directly in that code. That means that in the scenario you suggested, should rustbuild want tidy, it would simply rebuild it as-needed. Though this is obviously non-optimal, it would work.

For cleaning tools I looked into this and this is how it used to work. This previously worked because of "order only dependencies". That is, the "clean all tools" step didn't actually depend on libstd/libtest/librustc, but it always ran after those steps if they were present in the dependency graph.

It's worth pointing out that this is exactly what we do today, I believe. The old logic was wrong, though perhaps always worked out in practice since we ran tools as "after" dependencies of clean-tools, IIRC. That may have been sufficient, not sure, but it would've been about equivalent to the current state (without this PR). With this PR, we run CleanTools with the appropriate mode from the compile::{Std, Rustc, Test} steps. The additional logic that this PR introduces to that specific step's description should be unnecessary; I've pushed up a commit that removes that. However, tracking the fact that we have cleaned out the tools directory is still necessary, so that logic remains.

I'll look into the stamp file problem soon.

alexcrichton · 2017-09-26T14:08:03Z

@Mark-Simulacrum I'm a little confused then, isn't the ordering I posted above incorrect but possible today and with this PR?

That is, I could do something like:

ensure(tool_that_depends_on_std)
ensure(tool_that_depends_on_test)

The second step may kill the tool built in the first step if it builds libtest?

Mark-Simulacrum · 2017-09-27T00:57:53Z

Sure, yeah, that can happen -- I was more saying that I'd expect the workflow today to be that we run tools immediately after compilation. I could see how that might not be quite the case when looking at something like compiletest, though, e.g. with doc tests. Since we seem to be fine today though it's not a case I'm too worried about. We can (if necessary) keep a track of "built tools" per stage and rebuild all after clearing out, but it would probably be unnecessary and wasteful.

alexcrichton · 2017-09-27T16:40:10Z

Yeah I'd just personally prefer to not rely on "make sure you ensure things in the right order" as it's basically impossible for anyone to remember that and it's not always feasible depending on how the steps are orchestrated.

Mark-Simulacrum · 2017-09-27T16:47:28Z

I think implementing the copy approach for tools is a good one -- and it should resolve the problem of clearing out tools, too, I believe. stageN/bin is good, or does that have special significance for dist?

alexcrichton · 2017-09-27T16:51:30Z

Oh right yeah, I was thinking let's not use the folder where rustc goes for now, let's stick to another temporary dir perhaps?

bors · 2017-09-28T18:56:44Z

☔ The latest upstream changes (presumably #44785) made this pull request unmergeable. Please resolve the merge conflicts.

aidanhs · 2017-10-05T19:05:06Z

@Mark-Simulacrum just a ping on this PR, I note there's a merge conflict at minimum :)

aidanhs · 2017-10-12T15:28:49Z

Going to close this for now to keep the queue clean!

Mark-Simulacrum · 2017-10-16T18:02:00Z

@alexcrichton Updated this -- I've removed the directory scanning logic, I think I actually never needed it and just introduced it before realizing the build script problem. We can come back to that problem later if necessary; I think there might be an edge case if cargo is killed via ctrl-c but we can deal with it later down the road.

aidanhs · 2017-10-16T19:36:44Z

[00:04:43] travis_fold:end:stage0-tidy
�[0K
[00:04:43] travis_time:end:stage0-tidy:start=1508177191622429747,finish=1508177201331920288,duration=9709490541
�[0K
[00:04:43] tidy error: /checkout/src/bootstrap/tool.rs:119: line longer than 100 chars
[00:04:44] some tidy checks failed
[00:04:44] 
[00:04:44] 
[00:04:44] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "--no-vendor" "--quiet"
[00:04:44] expected success, got: exit code: 1
[00:04:44] 
[00:04:44] 
[00:04:44] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:44] Build completed unsuccessfully in 0:01:13
[00:04:44] Makefile:77: recipe for target 'tidy' failed
[00:04:44] make: *** [tidy] Error 1

alexcrichton · 2017-10-17T15:00:23Z

@bors: r+

Looks great!

bors · 2017-10-17T15:00:24Z

📌 Commit 39fa86c has been approved by alexcrichton

bors · 2017-10-18T02:42:19Z

⌛ Testing commit 39fa86c0bf56f6fa1f4a1a0ebeadf7fb80b5f0eb with merge f14ede0cc7faeb0f86ff060e4237c34957ec5cb3...

bors · 2017-10-18T03:42:09Z

💔 Test failed - status-travis

kennytm · 2017-10-18T07:30:25Z

Cannot build stage2-miri on x86_64-gnu-aux. I think this is irrelevant and might be caused by other PRs or miri itself, but it is not spurious either. I suggest a rebase before r+.

Building stage2 tool miri (x86_64-unknown-linux-gnu)
[00:55:21]  Downloading log_settings v0.1.1
[00:55:21]  Downloading byteorder v1.1.0
[00:55:21]  Downloading libc v0.2.30
[00:55:22]  Downloading backtrace-sys v0.1.12
[00:55:22]  Downloading gcc v0.3.53
[00:55:22]    Compiling log v0.3.8
[00:55:22]    Compiling byteorder v1.1.0
[00:55:22]    Compiling cfg-if v0.1.2
[00:55:22]    Compiling gcc v0.3.53
[00:55:22]    Compiling miri v0.1.0 (file:///checkout/src/tools/miri)
[00:55:23]    Compiling rustc-demangle v0.1.5
[00:55:23]    Compiling libc v0.2.30
[00:55:24]    Compiling utf8-ranges v1.0.0
[00:55:24]    Compiling void v1.0.2
[00:55:25]    Compiling unreachable v1.0.0
[00:55:25]    Compiling log_settings v0.1.1
[00:55:25]    Compiling thread_local v0.3.4
[00:55:25]    Compiling memchr v1.0.1
[00:55:26]    Compiling aho-corasick v0.6.3
[00:55:26]    Compiling regex v0.2.2
[00:55:30]    Compiling backtrace-sys v0.1.12
[00:55:38]    Compiling backtrace v0.3.3
[00:55:54]    Compiling rustc_miri v0.1.0 (file:///checkout/src/tools/miri/src/librustc_mir)
[00:55:54]    Compiling env_logger v0.4.3
[00:55:55] error[E0425]: cannot find function `get_vtable_methods` in module `rustc::traits`
[00:55:55]   --> /checkout/src/tools/miri/src/librustc_mir/interpret/traits.rs:57:40
[00:55:55]    |
[00:55:55] 57 |         let methods = ::rustc::traits::get_vtable_methods(self.tcx, trait_ref);
[00:55:55]    |                                        ^^^^^^^^^^^^^^^^^^ did you mean `vtable_methods`?
[00:55:55] 
[00:55:55] error[E0425]: cannot find function `get_vtable_methods` in module `rustc::traits`
[00:55:55]   --> /checkout/src/tools/miri/src/librustc_mir/interpret/traits.rs:73:45
[00:55:55]    |
[00:55:55] 73 |         for (i, method) in ::rustc::traits::get_vtable_methods(self.tcx, trait_ref).enumerate() {
[00:55:55]    |                                             ^^^^^^^^^^^^^^^^^^ did you mean `vtable_methods`?
[00:55:55] 
[00:55:56] error[E0061]: this function takes 1 parameter but 2 parameters were supplied
[00:55:56]     --> /checkout/src/tools/miri/src/librustc_mir/interpret/eval_context.rs:2419:45
[00:55:56]      |
[00:55:56] 2419 |     let vtbl = tcx.trans_fulfill_obligation(DUMMY_SP, ty::Binder(trait_ref));
[00:55:56]      |                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected 1 parameter
[00:55:56] 
[00:55:57] error: aborting due to 3 previous errors
[00:55:57] 
[00:55:57] error: Could not compile `rustc_miri`.
[00:55:57] 
[00:55:57] To learn more, run the command again with --verbose.
[00:55:57] This failure is expected (see `src/tools/toolstate.toml`)
[00:55:57] thread 'main' panicked at 'failed to copy `/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/miri` to `/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools-bin/miri`: the source path is not an existing regular file', /checkout/src/bootstrap/util.rs:44:8
[00:55:57] note: Run with `RUST_BACKTRACE=1` for a backtrace.

We copy built tool binaries into a dedicated directory to avoid deleting them, stageN-tools-bin. These aren't ever cleared out by code, since there should be no reason to do so, and we'll simply overwrite them as necessary. When clearing out the stageN-{std,rustc,tools} directories, make sure to delete both Cargo directories -- per-target and build scripts. This ensures that changing libstd doesn't cause problems due to build scripts not being rebuilt, even though they should be.

Mark-Simulacrum · 2017-10-18T22:31:39Z

I think clippy might be broken? I can't really tell. I've rebased, going to wait on travis though.

Mark-Simulacrum · 2017-10-19T00:07:55Z

@alexcrichton I don't understand how I've managed to break clippy in a PR that touches none of the related code. Did I do something wrong?

alexcrichton · 2017-10-19T14:04:53Z

wut

alexcrichton · 2017-10-19T14:06:47Z

Oh that's expected, the real error is:

[00:49:25] thread 'main' panicked at 'failed to copy `/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/clippy-driver` to `/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools-bin/clippy-driver`: the source path is not an existing regular file', /checkout/src/bootstrap/util.rs:44:8

This makes it mandatory for other steps to have to handle the potential failure instead of failing in an odd way later down the road.

alexcrichton · 2017-10-20T15:19:36Z

@bors: r+

bors · 2017-10-20T15:19:36Z

📌 Commit 686c101 has been approved by alexcrichton

bors · 2017-10-20T17:36:46Z

⌛ Testing commit 686c101 with merge b633341...

Make sure to clear out the stageN-{rustc,std,tools} directories. We copy built tool binaries into a dedicated directory to avoid deleting them, stageN-tools-bin. These aren't ever cleared out by code, since there should be no reason to do so, and we'll simply overwrite them as necessary. When clearing out the stageN-{std,rustc,tools} directories, make sure to delete both Cargo directories -- per-target and build scripts. This ensures that changing libstd doesn't cause problems due to build scripts not being rebuilt, even though they should be. Fixes #44739.

bors · 2017-10-20T20:36:57Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing b633341 to master...

rust-highfive assigned alexcrichton Sep 23, 2017

carols10cents added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 25, 2017

Mark-Simulacrum force-pushed the correct-deps branch from 44512b7 to e2e8e36 Compare September 26, 2017 01:02

aidanhs closed this Oct 12, 2017

Mark-Simulacrum reopened this Oct 16, 2017

Mark-Simulacrum force-pushed the correct-deps branch from e2e8e36 to 5951df6 Compare October 16, 2017 17:59

Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 16, 2017

Mark-Simulacrum force-pushed the correct-deps branch from 5951df6 to 39fa86c Compare October 16, 2017 23:34

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 17, 2017

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 18, 2017

Mark-Simulacrum force-pushed the correct-deps branch from 39fa86c to 58252a3 Compare October 18, 2017 15:50

Mark-Simulacrum force-pushed the correct-deps branch from 58252a3 to 0fcd3e7 Compare October 18, 2017 22:31

Make tools which may not build return Option.

686c101

This makes it mandatory for other steps to have to handle the potential failure instead of failing in an odd way later down the road.

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 20, 2017

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 20, 2017

bors merged commit 686c101 into rust-lang:master Oct 20, 2017

bors mentioned this pull request Oct 20, 2017

Fix rustbuild --color conflict when building on Travis outside of Docker #45304

Merged

Mark-Simulacrum deleted the correct-deps branch November 14, 2017 23:52

SimonSapin mentioned this pull request Apr 15, 2018

"found possibly newer version of crate std which cc depends on" while building rustc #49979

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure to clear out the stageN-{rustc,std,tools} directories. #44792

Make sure to clear out the stageN-{rustc,std,tools} directories. #44792

Mark-Simulacrum commented Sep 23, 2017 •

edited

Loading

alexcrichton commented Sep 25, 2017

Mark-Simulacrum commented Sep 25, 2017

alexcrichton commented Sep 25, 2017

Mark-Simulacrum commented Sep 26, 2017

alexcrichton commented Sep 26, 2017

Mark-Simulacrum commented Sep 27, 2017

alexcrichton commented Sep 27, 2017

Mark-Simulacrum commented Sep 27, 2017

alexcrichton commented Sep 27, 2017

bors commented Sep 28, 2017

aidanhs commented Oct 5, 2017

aidanhs commented Oct 12, 2017

Mark-Simulacrum commented Oct 16, 2017

aidanhs commented Oct 16, 2017

alexcrichton commented Oct 17, 2017

bors commented Oct 17, 2017

bors commented Oct 18, 2017

bors commented Oct 18, 2017

kennytm commented Oct 18, 2017

Mark-Simulacrum commented Oct 18, 2017

Mark-Simulacrum commented Oct 19, 2017

alexcrichton commented Oct 19, 2017

alexcrichton commented Oct 19, 2017

alexcrichton commented Oct 20, 2017

bors commented Oct 20, 2017

bors commented Oct 20, 2017

bors commented Oct 20, 2017

Make sure to clear out the stageN-{rustc,std,tools} directories. #44792

Make sure to clear out the stageN-{rustc,std,tools} directories. #44792

Conversation

Mark-Simulacrum commented Sep 23, 2017 • edited Loading

alexcrichton commented Sep 25, 2017

Mark-Simulacrum commented Sep 25, 2017

alexcrichton commented Sep 25, 2017

Mark-Simulacrum commented Sep 26, 2017

alexcrichton commented Sep 26, 2017

Mark-Simulacrum commented Sep 27, 2017

alexcrichton commented Sep 27, 2017

Mark-Simulacrum commented Sep 27, 2017

alexcrichton commented Sep 27, 2017

bors commented Sep 28, 2017

aidanhs commented Oct 5, 2017

aidanhs commented Oct 12, 2017

Mark-Simulacrum commented Oct 16, 2017

aidanhs commented Oct 16, 2017

alexcrichton commented Oct 17, 2017

bors commented Oct 17, 2017

bors commented Oct 18, 2017

bors commented Oct 18, 2017

kennytm commented Oct 18, 2017

Mark-Simulacrum commented Oct 18, 2017

Mark-Simulacrum commented Oct 19, 2017

alexcrichton commented Oct 19, 2017

alexcrichton commented Oct 19, 2017

alexcrichton commented Oct 20, 2017

bors commented Oct 20, 2017

bors commented Oct 20, 2017

bors commented Oct 20, 2017

Mark-Simulacrum commented Sep 23, 2017 •

edited

Loading