add another exception

[RalfJung]

This failed with the same error in both runs in rust-lang/rust#104429.

Doesn't crater run both builds in the same clean environment? We seem to have a bunch of cases of "baseline always works but try build always fails". It almost seems like the 'try' run can see artifacts created by the baseline, and that breaks quite a few poorly written build scripts out there.

[est31]

Doesn't crater run both builds in the same clean environment? We seem to have a bunch of cases of "baseline always works but try build always fails".

It doesn't, that's what I was trying to say in this comment. The environment is shared between before and after builds.

It almost seems like the 'try' run can see artifacts created by the baseline, and that breaks quite a few poorly written build scripts out there.

Yeah those are bugs in the build scripts.

[RalfJung]

Sounds like it'd be a good idea to do both runs in the same clean environment? Otherwise this will systematically look like a regression. And it's not like build artifacts can be shared anyway since the compiler changes.

[est31]

I think the problem is that the CARGO_HOME environment (where the crates.io downloads are) is shared across the entire worker (or maybe even all workers? not 100% sure the CARGO_TARGET path has worker number in it the CARGO_HOME path does not). That means that if you create a totally clean environment for each and every build, you'd suddenly download libc tens to hundreds of thousands of times, instead of just once per worker or once at all.

If you had two environments instead, one for all jobs from the try build and one for all jobs from the master build, then the first and second build wouldn't interfere. However, you'd still have interference between crates. Like what if you test libfoobar from crates.io and then libfoobar from github. Both depend on libfoobar-sys from crates.io that has a buggy build.rs. The libfoobar from crates.io build will succeed while the libfoobar from github build will fail.

However, I think there is hope.

• There is already a cargo fetch step that's separate from the actual build, and it's executed outside of docker before the docker create command.
• Maybe one could just simply pass the CARGO_HOME directory to the docker invocation as read-only, and then inside the docker container, mount an overlay file system to turn that into read/write. Then build.rs scripts can do whatever they want, their changes will be isolated to the local build.
• I am no docker expert, but this seems to be a guide for doing this with docker.

[RalfJung]

The state that is "polluted" here should be in the target directory, not CARGO_HOME. So preserving CARGO_HOME seems fine?

[est31]

Some of the pollution arises from the shared target directory, yes.

It seems that rustc versions close enough produce the same OUT_DIR (which is in target/). I've tested with a println!("{:?}", std::env::var("OUT_DIR")); in a build.rs, with today's nightly and nightly-2023-01-26, they are the same even though they are days apart. Meanwhile, beta, nightly, and stable have pairwise different hashes in OUT_DIR.

But OUT_DIR sharing is not the entire problem. Some build scripts also modify directories in the source code checkout, which can be found in CARGO_HOME. This is of course quite horrible, as cargo has zero sense of tracking of that, but it's still done. Often times, C build systems put the .o files right next to the .c files for example. A well behaved build.rs script would copy the files to OUT_DIR and do the build there, but it's simpler to just issue make after cd'ing into the source directory...

For example, from this crater run, this build.rs failed at the linked line after attempting to remove an assets/ directory that is a subdirectory of the source code checkout. It's the same symlink issue that binaryninja had too, and the symlink issue is probably an artifact of the environment, but the core issue exists that it shouldn't modify the source checkout. Cargo does not have logic to re-download stuff from there. But it's not the only build system that does such stuff. Years ago, during my f-droid packaging days, I've compiled android apps that would mess up the android ndk in such a way that you'd have to re-download it if you wanted other stuff to compile...

But yes, good point, the separation operation needs to be done both for OUT_DIR and CARGO_HOME. I guess for OUT_DIR similar concerns arise, maybe in fact crater is not rebuilding every possible crate out there each time it appears as dependency, only if something in the crate's DAG changes deep enough to affect the crate's build. This would occur way more often than needing a completely new version from crates.io so it would explain why you can often see Compiling foo v0.1.0... in the logs but way more rarely Downloading foo v0.1.0. I'm applying black box analysis here because the crater source code is not easy to walk through :).

[RalfJung]

@Mark-Simulacrum friendly reminder that this PR has been sitting here for half a year. :)

[Mark-Simulacrum]

@bors r+

[bors]

📌 Commit 5dbc79b has been approved by Mark-Simulacrum

It is now in the queue for this repository.

[bors]

⌛ Testing commit 5dbc79b with merge 0aca964...

[bors]

💔 Test failed - checks-actions

[RalfJung]

Seems unrelated to my PR

error[E0635]: unknown feature `proc_macro_span_shrink`
  --> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/proc-macro2-1.0.54/src/lib.rs:92:30
   |
92 |     feature(proc_macro_span, proc_macro_span_shrink)
   |                              ^^^^^^^^^^^^^^^^^^^^^^

[Mark-Simulacrum]

Yeah, probably needs a dependency bump. I'll try to get to it sooner than I responded to your poke here...

[RalfJung]

@Mark-Simulacrum some PR has landed since the above CI failure, so maybe retrying this one works now?

[Mark-Simulacrum]

@bors retry

[bors]

⌛ Testing commit 5dbc79b with merge 5196682...

[bors]

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 5196682 to master...