Install binaries to the miri toolchain's sysroot

[saethlin]

The default install produces this behavior:

$ cargo +miri miri --version
miri 0.1.0 (0ba1f4a0 2023-03-05)
$ cargo +nightly miri --version
miri 0.1.0 (0ba1f4a0 2023-03-05)

Which is not good. We've effectively erased the toolchain selection, and users may reasonably conclude that their rustup install is broken.

After this change, we now get this:

$ cargo +miri miri --version
miri 0.1.0 (0ba1f4a0 2023-03-05)
$ cargo +nightly miri --version
miri 0.1.0 (f63ccaf 2023-03-06)

Thanks @jyn514 who all but wrote this for me.

[jyn514]

Oh, I forgot cargo install works with arbitrary directories like that 😄 that's even nicer than the bash I was expecting.

[saethlin]

That is very strange. I can't reproduce the CI failure on my desktop, and I also can't reproduce it in a fresh ubuntu:latest container. Here's my shell history from that container:

    1  cd
    2  git
    3  apt-get update
    4  apt-get install -y git gcc make
    5  apt-get install -y build-essential
    6  git clone https://github.com/saethlin/miri
    7  cd miri/
    8  curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh
    9  apt-get install -y curl
   10  curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh
   11  ./miri toolchain
   12  apt-get install -y python3
   13  ./miri toolchain
   14  source ~/.cargo/env 
   15  cargo install rustup-toolchain-install-master
   16  apt-get install -y pkg-config
   17  cargo install rustup-toolchain-install-master
   18  apt-get install -y libssl-devel
   19  apt-get install -y libssl-deve
   20  apt-get install -y libssl-dev
   21  apt-get install -y pkg-config
   22  cargo install rustup-toolchain-install-master
   23  ./miri toolchain
   24  ./miri test
   25  python3 test-cargo-miri/run-test.py 
   26  ./miri install
   27  python3 test-cargo-miri/run-test.py 
   28  history

[jyn514]

@saethlin is there a way to see what command it's trying to run that doesn't exist? I see echo 'build.rustc-wrapper = "thisdoesnotexist"' on the line above which looks pretty suspicious.

[RalfJung]

Ah, nice idea.

Is it even using the right binaries on CI still?

[rbtcollins]

Looks sensible to me (wearing my rustup hat).

[saethlin]

Is it even using the right binaries on CI still?

Yes that was the problem. CI caches the contents of ~/.cargo/bin/, so even though we are no longer installing there, we get a non-functional miri and cargo-miri dropped in there by the CI cache.

I don't know what to do about this. Perhaps we remove that directory from the cache, land this PR, then add them back? Just to prove that this PR works and cache is the problem, I deleted the MacOS cache via GitHub and re-ran.

[saethlin]

What is this suppposed to test?

miri/ci.sh

Lines 61 to 66 in 0ba1f4a

	# Some environment setup that attempts to confuse the heck out of cargo-miri.
	if [ "$HOST_TARGET" = x86_64-unknown-linux-gnu ]; then
	# These act up on Windows (`which miri` produces a filename that does not exist?!?),
	# so let's do this only on Linux. Also makes sure things work without these set.
	export RUSTC=$(which rustc)
	export MIRI=$(which miri)

The comment just says "confuse" and I'm not even sure what could be confusing about this- we set RUSTC and MIRI to the executables we're trying to run, right?

[RalfJung]

Yeah but them being set caused bugs in the past... see #2430.

[RalfJung]

CI caches the contents of ~/.cargo/bin/

Is that still even useful? We did that to cache xargo, but we no longer use xargo.

EDIT: Ah, it caches rustup-toolchain-install-master.
Maybe you can add a step after the cache that does rm ~/.cargo/bin/{miri,cargo-miri}, or does that mess with rustup too much?

[saethlin]

see #2430.

I see the motivation to test build.rustc-wrapper there, that makes sense. Maybe I'm just being dense, but I still don't understand testing what happens when RUSTC and MIRI are set?

[RalfJung]

Setting RUSTC_WRAPPER and RUSTC leads to very interesting cargo behavior (the value of the RUSTC env var is passed to the RUSTC_WRAPPER as its first argument). So having RUSTC set is quite important, since Miri made some wrong assumptions here for years (namely that the first argument to RUSTC_WRAPPER is always rustc). And then while I was at it I just set MIRI as well for good measure.

So if setting RUSTC now causes CI failure, that's not a good sign. What happens when you bring back those env vars?

[saethlin]

So if setting RUSTC now causes CI failure, that's not a good sign. What happens when you bring back those env vars?

If I bring back only RUSTC, we fail CI because we get a warning and thus unexpected stdout. If I bring back both RUSTC and MIRI, while fixing the path for MIRI to be the interpreter we are trying to run, everything passes.

[RalfJung]

Nice, so looks like this approach works!

I think this means that the documentation at the top of the file, and in the readme, should be updated? The part about the toolchain staying the same is no longer needed, the docs should say where the binaries go, and we can remove the docs about how to reset rustup after ./miri install.

[RalfJung]

Thanks! This elegantly solves an ancient issue when working on Miri. :)
@bors r+

[bors]

📌 Commit e56eb12 has been approved by RalfJung

It is now in the queue for this repository.

[bors]

⌛ Testing commit e56eb12 with merge d3e64f7...

[saethlin]

@bors r=RalfJung

[bors]

📌 Commit c160120 has been approved by RalfJung

It is now in the queue for this repository.

[bors]

⌛ Testing commit c160120 with merge dbf56c1...

[bors]

💔 Test failed - checks-actions

[saethlin]

Gah. I guess I was bamboozled by the CI cache after all because I never took your advice to add a step to delete the old binaries. I'll debug this tomorrow.

[RalfJung]

Now your deletion code deletes the rustup-provided cargo-miri. We probably need a rustup update as well.

Or maybe the better idea is the key (and restore-keys) such that we just don't pick up the old cache any more.

[saethlin]

When I've tried it, rustup update will only rewrite the rustup shims if it actually does an update.

Debugging this is also so frustrating because I can't coax any of the failures out locally. Even HOST_TARGET=x86_64-unknown-linux-gnu ./ci.sh works fine locally.

[saethlin]

Yep, changing the cache key seems to get CI working again.

[RalfJung]

When I've tried it, rustup update will only rewrite the rustup shims if it actually does an update.

I specifically tested this before adding that to the readme... strange. Anyway we'll not have to rely on it for much longer. ;)

[RalfJung]

@bors r+

[bors]

📌 Commit ee9fbba has been approved by RalfJung

It is now in the queue for this repository.

[bors]

⌛ Testing commit ee9fbba with merge 42799c7...

[bors]

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 42799c7 to master...