rust-lang / miri

An interpreter for Rust's mid-level intermediate representation
Apache License 2.0
4.17k stars 320 forks source link

sysroot is being rebuilt whenever I save a file #3527

Closed RustyYato closed 2 months ago

RustyYato commented 2 months ago

Every time I save a file in my editor, then run cargo miri test, miri decides to completely rebuild the sysroot!

$ cargo miri test
Preparing a sysroot for Miri (target: x86_64-unknown-linux-gnu)... # this takes a while
# in editor, save some test file (even with no changes)
$ cargo miri test
Preparing a sysroot for Miri (target: x86_64-unknown-linux-gnu)... # this takes a while again, but the sysroot shouldn't have changed!
$ rustc --version
rustc 1.80.0-nightly (a8a1d3a77 2024-04-29)

I poked around in the sysroot, and I noticed that the hash is changing each time it gets rebuilt. So maybe that's the issue? Is there a way I can force the sysroot to be reused to work around this bug?

RustyYato commented 2 months ago

I suspect this has something to do with rust-analyzer not playing nicely with miri, since if I simply touch or append to the file without using my editor, then miri doesn't rebuild the sysroot.

I'm building rust-analyzer from source, at commit a5feb4f05f09adca661c869b1bf2324898cbaa43, with some logs disabled.


edit: actually this is back again... maybe it's still a bug in rust-analyzer tho... It looks like this was a bug with rust-analyzer, since after updating to the latest commit (49e502b277a8126a9ad10c802d1aaa3ef1a280ef), miri isn't misbehaving

saethlin commented 2 months ago

The sysroot hash is based on a hash of the modified time and length of each file in the sysroot source tree.

Are you changing the modified time of files in the sysroot?

RustyYato commented 2 months ago

The sysroot hash is based on a hash of the modified time and length of each file in the sysroot source tree.

Are you changing the modified time of files in the sysroot?

I'm not, and I checked the modified time, and it's the time I installed it earlier today. (I updated to the latest nightly earlier today).

RustyYato commented 2 months ago

Hmm, after some further investigation, it looks like the rustlib/src/rust/library/core/target/ directory was modified afterwards... I'm not sure why though. Maybe this does have to do with rust-analyzer. Is the target directory ignored by miri?

RalfJung commented 2 months ago

Hm, yeah we should probably exclude "target" directories from hashing. Not sure why something creates a tatget dir in the source dir for you, usually the source dir should be immutable, but anyway it cannot affect the resulting sysroot.

saethlin commented 2 months ago

I don't think we should add any logic that tries to exclude directories because they are called target. We added this hashing so that the sysroot is rebuilt when it changes, and a magic excluded directory name seems like a way to cause subtle problems in the future. Ideally we'd restrict our hashing to files which Cargo or rustc can tell us are part of the build, but I don't think such an API exists.

I wouldn't mind doing something to cooperate better with rust-analyzer here, clearly some tool was confused. Perhaps this was a bug in rust-analyzer that was noticed and quietly fixed upstream? There is no good reason to be writing in the sysroot's source tree.

RalfJung commented 2 months ago

I think I have run into a similar issue in the past when I set MIRI_LIB_SRC to a local rustc checkout and also did some manual cargo invocations in that folder. "target" is a very standard directory in Rust so it seems justified to me to just exclude it.

But it would indeed also be good to figure out why something is writing to the sysroot dir. I am confused by the last few messages, does this issue still occur with latest RA or not?

RustyYato commented 2 months ago

I tested this again today, and it looks like this isn't happening anymore. So it was likely RA causing some issues. I'll open an issue there if I run into this problem again.... Sorry for the noise :bow:

RalfJung commented 2 months ago

All right, let's close this then for now.