rustc: Remove `Session::dep_graph` by alexcrichton · Pull Request #44502 · rust-lang/rust

alexcrichton · 2017-09-11T19:12:14Z

This commit removes the dep_graph field from the Session type according to
issue #44390. Most of the fallout here was relatively straightforward and the
prepare_session_directory function was rejiggered a bit to reuse the results
in the later-called load_dep_graph function.

Closes#44390

rust-highfive · 2017-09-11T19:12:18Z

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton · 2017-09-11T19:42:33Z

r? @michaelwoerister

michaelwoerister · 2017-09-12T08:55:29Z

Looks good to me, but something seems to have gone wrong somewhere because tests are failing.

michaelwoerister · 2017-09-12T09:05:10Z

I've looked over the changes again and I don't see anything that should make a semantic difference. It's interesting that all failing tests are some kind of cross-crate scenarios.

nikomatsakis · 2017-09-12T22:08:03Z

Problem is that MetaData(a::function1) is changing (when it did not before).

bors · 2017-09-13T08:29:09Z

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

michaelwoerister · 2017-09-13T13:55:48Z

So, it seems that moving incr-comp-session-dir initialization up to an earlier point suffices to make these tests fail. Don't know why yet.

michaelwoerister · 2017-09-13T14:10:31Z

OK, so the problem is that we are reading the crate_disambiguator from the Session before it is initialized here:

rust/src/librustc_driver/driver.rs

Line 627 in 824952f

*sess.crate_disambiguator.borrow_mut() = Symbol::intern(&compute_crate_disambiguator(sess));

We end up with a wrong directory for the crate and therefore can't find the metadata hashes later. When you fix this, @alexcrichton, could you also make sure that we get an ICE if someone tries to access the crate_disambiguator before it is initialized?

alexcrichton · 2017-09-13T14:14:56Z

Oh wow thanks for helping debug that @michaelwoerister! I'll fix that and make an Ice

alexcrichton · 2017-09-13T14:30:33Z

@bors: r=michaelwoerister

bors · 2017-09-13T14:30:34Z

📌 Commit 23c531b has been approved by michaelwoerister

alexcrichton · 2017-09-13T15:16:08Z

@bors: r=michaelwoerister

bors · 2017-09-13T15:16:09Z

📌 Commit 0bf82f5 has been approved by michaelwoerister

alexcrichton · 2017-09-13T17:41:50Z

@bors: r=michaelwoerister

bors · 2017-09-13T17:41:51Z

📌 Commit 9212b69 has been approved by michaelwoerister

alexcrichton · 2017-09-13T18:15:49Z

@bors: r=michaelwoerister

bors · 2017-09-13T18:15:49Z

📌 Commit 7e983d6 has been approved by michaelwoerister

bors · 2017-09-14T08:04:23Z

⌛ Testing commit 7e983d6a178cfb75461ff498f5f02b03e3313ac4 with merge a2e806ad5f9636471725b35f353c8188b04b8bac...

bors · 2017-09-14T09:18:24Z

💔 Test failed - status-travis

kennytm · 2017-09-14T09:44:06Z

Cannot test stage1-librustc, legit. At least one build_session_ call not fixed.

[01:06:59] Compiling rustc_driver v0.0.0 (file:///checkout/src/librustc_driver)
[01:07:01] error[E0061]: this function takes 4 parameters but 5 parameters were supplied
[01:07:01] --> /checkout/src/librustc_driver/test.rs:108:40
[01:07:01] |
[01:07:01] 108 | let sess = session::build_session_(options,
[01:07:01] | ^^^^^^^ expected 4 parameters
[01:07:01]
[01:07:02] error: aborting due to previous error
[01:07:02]
[01:07:02] error: Could not compile `rustc_driver`.
[01:07:02] warning: build failed, waiting for other jobs to finish...
[01:08:24] error: build failed

This commit removes the `dep_graph` field from the `Session` type according to issue rust-lang#44390. Most of the fallout here was relatively straightforward and the `prepare_session_directory` function was rejiggered a bit to reuse the results in the later-called `load_dep_graph` function. Closesrust-lang#44390

alexcrichton · 2017-09-14T17:41:13Z

@bors: r=michaelwoerister

bors · 2017-09-14T17:41:14Z

📌 Commit 1cf956f has been approved by michaelwoerister

bors · 2017-09-14T19:37:17Z

⌛ Testing commit 1cf956f with merge 2d288a5...

…elwoerister rustc: Remove `Session::dep_graph` This commit removes the `dep_graph` field from the `Session` type according to issue #44390. Most of the fallout here was relatively straightforward and the `prepare_session_directory` function was rejiggered a bit to reuse the results in the later-called `load_dep_graph` function. Closes#44390

bors · 2017-09-15T00:29:00Z

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 2d288a5 to master...

michaelwoerister · 2017-09-15T10:28:58Z

Awesome!

…tebank Don't panic when failing to initialize incremental directory. This removes a panic when rustc fails to initialize the incremental directory. This can commonly happen on various filesystems that don't support locking (often various network filesystems). Panics can be confusing and scary, and there are already plenty of issues reporting this. This has been panicking since 1.22 due to I think rust-lang#44502 which was a major rework of how things work. Previously, things were simpler and the [`load_dep_graph`](https://github.com/rust-lang/rust/blob/1.21.0/src/librustc_incremental/persist/load.rs#L43-L65) function would emit an error and then continue on without panicking. With 1.22, [`load_dep_graph`](https://github.com/rust-lang/rust/blob/1.22.0/src/librustc_incremental/persist/load.rs#L44) was changed so that it assumes it can load the data without errors. Today, the problem is that it calls [`prepare_session_directory`](https://github.com/rust-lang/rust/blob/fbf1b1a7193cda17008ab590e06ad28d9924023b/compiler/rustc_interface/src/passes.rs#L175-L179) and then immediately calls `garbage_collect_session_directories` which will panic since the session is `IncrCompSession::NotInitialized`. The solution here is to have `prepare_session_directory` return an error that must be handled so that compilation stops if it fails. Some other options: * Ignore directory lock failures. * Print a warning on directory lock failure, but otherwise continue with incremental enabled. * Print a warning on directory lock failure, and disable incremental. * Provide a different locking mechanism. Cargo ignores lock errors if locking is not supported, so that would be a precedent for the first option. These options would require quite a bit more changes, but I'm happy to entertain any of them, as I think they all have valid justifications. There is more discussion on the many issues where this is reported: rust-lang#49773, rust-lang#59224, rust-lang#66513, rust-lang#76251. I'm not sure if this can be considered closing any of those, though, since I think there is some value in discussing if there is a way to avoid the error altogether. But I think it would make sense to at least close all but one to consolidate them.

rust-highfive assigned arielb1 Sep 11, 2017

michaelwoerister mentioned this pull request Sep 11, 2017
incr.comp.: Do not hard-wire DepGraph into Session and CrateStore. #44390
Closed
7 tasks

rust-highfive assigned michaelwoerister and unassigned arielb1 Sep 11, 2017

alexcrichton force-pushed the remove-session-dep-graph branch 4 times, most recently from fca0733 to 789af95Compare September 12, 2017 04:09

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

alexcrichton force-pushed the remove-session-dep-graph branch from 789af95 to 23c531bCompare September 13, 2017 14:30

alexcrichton force-pushed the remove-session-dep-graph branch from 23c531b to 0bf82f5Compare September 13, 2017 15:15

alexcrichton force-pushed the remove-session-dep-graph branch 2 times, most recently from 6da258a to 9212b69Compare September 13, 2017 17:41

alexcrichton force-pushed the remove-session-dep-graph branch from 9212b69 to 7e983d6Compare September 13, 2017 18:15

alexcrichton force-pushed the remove-session-dep-graph branch from 7e983d6 to 1cf956fCompare September 14, 2017 17:41

bors mentioned this pull request Sep 14, 2017
Refactor translation unit partitioning/collection as a query #44529
Merged

bors merged commit 1cf956f into rust-lang:masterSep 15, 2017

alexcrichton deleted the remove-session-dep-graph branch September 15, 2017 14:29

ehuss mentioned this pull request May 25, 2021
Don't panic when failing to initialize incremental directory. #85698
Merged

Uh oh!

Uh oh!

Conversation

alexcrichton commented Sep 11, 2017

Uh oh!

rust-highfive commented Sep 11, 2017

Uh oh!

alexcrichton commented Sep 11, 2017

Uh oh!

michaelwoerister commented Sep 12, 2017

Uh oh!

michaelwoerister commented Sep 12, 2017

Uh oh!

nikomatsakis commented Sep 12, 2017

Uh oh!

bors commented Sep 13, 2017

Uh oh!

michaelwoerister commented Sep 13, 2017

Uh oh!

michaelwoerister commented Sep 13, 2017

Uh oh!

alexcrichton commented Sep 13, 2017

Uh oh!

alexcrichton commented Sep 13, 2017

Uh oh!

bors commented Sep 13, 2017

Uh oh!

alexcrichton commented Sep 13, 2017

Uh oh!

bors commented Sep 13, 2017

Uh oh!

alexcrichton commented Sep 13, 2017

Uh oh!

bors commented Sep 13, 2017

Uh oh!

alexcrichton commented Sep 13, 2017

Uh oh!

bors commented Sep 13, 2017

Uh oh!

bors commented Sep 14, 2017

Uh oh!

bors commented Sep 14, 2017

Uh oh!

kennytm commented Sep 14, 2017

Uh oh!

alexcrichton commented Sep 14, 2017

Uh oh!

bors commented Sep 14, 2017

Uh oh!

bors commented Sep 14, 2017

Uh oh!

bors commented Sep 15, 2017

Uh oh!

michaelwoerister commented Sep 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants