miri component gets shipped even though the tests fail

[RalfJung]

Looking at https://rust-lang.github.io/rustup-components-history/, it seems like Miri is getting shipped as a rustup component even though the tests fail. Unfortunately, Miri is currently entirely usable, so it'd probably be better if it did not get shipped in this state. Is that something we can do?

Cc @oli-obk

[oli-obk]

maybe we can make the miri toolstate always go to build-fail even if it technically is just test-fail

[RalfJung]

@rust-lang/infra what would be the best place to change this? I think I'd prefer Miri not to be shipped at all when the tests fail; I'd rather people stick to an old-but-working Miri than to a new-but-broken one.

I found https://github.com/rust-lang/rust/blob/master/src/ci/docker/x86_64-gnu-tools/checktools.sh but that does not seem to decide if a component gets built for distribution or not.

[oli-obk]

rust/src/bootstrap/dist.rs

Lines 1368 to 1369 in 6312b89

	// We expect miri to build, because we've exited this step above if tool
	// state for miri isn't testing.

seems to suggest that it is supposed to work as you describe that it should be working. Since it obviously doesn't and I can't see how that comment could be true from looking at the source, I'm guessing this is some refactoring fallout. I'll dig a little into git blame

[oli-obk]

I found it. The comment was added for rls in 686c101 but then the code making the comment true was removed later

[RalfJung]

See #61656 (comment) for why this is hard to fix: the dist builders don't even run the tool tests, so they cannot know if the tests pass. :/

Also, that's probably for the better actually, as the "tools" builder sets --cfg miri to be more Miri-friendly and we don't want that in the release artifacts.

[RalfJung]

One (crazy?) thing we could so is have the dist builders query the toolstate repo to figure out if the tests are passing...

[kennytm]

That will either retrieve an outdated toolstate or will need to block until the toolstate is uploaded ...

[RalfJung]

Without blocking it would lag behind by 1 merged PR, yes.

[mati865]

@RalfJung would it be possible to add some kind of build assert so Miri fails on dist builders when Rust internals change too much?

[RalfJung]

Well dist builders already know when Miri fails to build because internals change too much. The issue is things that make Miri still compile but break the test suite.

[RalfJung]

@kennytm would it be possible to at least change the logic that opens issues such that for Miri, it opens an issue like #60533 when the tests fail? I could also try to implement that if you point me to the right place.

So far we have had zero cases of Miri tests spuriously failing.

[mati865]

That should be easy to do:

rust/src/tools/publish_toolstate.py

Line 152 in 0e4a56b

if build_failed:

[RalfJung]

Gave it a shot at #61938

[RalfJung]

@Centril had an interesting idea: can't we do the "don't ship broken tools" as part of a post-processing step? We anyway do some little amount of work when a commit hits master (as opposed to auto), such as actually updating the toolstate repo. So maybe that could also either delete the broken components from the S3 bucket, or else generate a file to be put into the S3 bucket that says which of the components are really there so that the broken ones can be considered "not there"?

@kennytm do you think something like that would be possible?

[RalfJung]

Thinking about this again, probably the most natural place to do this is in the code that turns CI artifacts into nightly releases. That could "just" also check the toolstate database for the commit it is promoting, and then (possibly only for certain tools) not add them to the nightly if their toolstate is not green.

[RalfJung]

This is biting us again right now, where CI knows that Miri is broken but we ship the component anyway, leading to CI failures in downstream projects that have Miri in their CI.

[RalfJung]

For another case where this issue got quite painful, see thomcc/rust-typed-arena#30.

[RalfJung]

My PR is getting reverted because I was incorrectly assuming that all commits have their toolstate recorded in the toolstate repo. Reopening.

[RalfJung]

FTR, for testing build-manifest, I downloaded some artifacts:

mkdir dist && cd dist
wget 'https://s3-us-west-1.amazonaws.com/rust-lang-ci2/rustc-builds/5670d048c0f88af9976b5505c7853b23dd06770d/rust-nightly-x86_64-unknown-linux-gnu.tar.gz'
wget 'https://s3-us-west-1.amazonaws.com/rust-lang-ci2/rustc-builds/5670d048c0f88af9976b5505c7853b23dd06770d/miri-nightly-x86_64-unknown-linux-gnu.tar.gz'

then I edited my config.toml:

# somewhere in the existing part
channel = "nightly"

# add at the bottom
[dist]
sign-folder = "dist"
upload-addr = "dist"

and finally I ran

BUILD_MANIFEST_DISABLE_SIGNING=1 ./x.py dist hash-and-sign

This produces stuff in build/dist.

[RalfJung]

So @pietroalbini @kennytm looks like we have logic in change_toolstate that specifically does not commit to the toolstate repo if nothing changed:

rust/src/ci/docker/x86_64-gnu-tools/checktools.sh

Lines 112 to 113 in 5670d04

	if python2.7 "$CHECK_NOT" "$OS" "$TOOLSTATE_FILE" "_data/latest.json" changed; then
	echo 'Toolstate is not changed. Not updating.'

Unfortunately, this means that we cannot easily fetch the toolstate of the to-be-produced release in build-manifest, which caused my original approach to fail.

Would it be okay for you to change this logic to record the toolstate of all commits in the toolstate repo? Or would you prefer if I try to find another approach for solving this issue? Right now I am somewhat out of ideas though if we cannot even use the toolstate repo to communicate this information.

[mati865]

Wouldn't it be simpler to save list of "good" components to something like available_components and upload it alongside tarballs?
Running curl 'https://s3-us-west-1.amazonaws.com/rust-lang-ci2/rustc-builds/<hash>/available_components' would show what you need.

[RalfJung]

I guess we could upload the two JSON files from the two tool runners somehow? Not sure which would be the right places to hook for that, though.

[RalfJung]

Status: blocked on #65274 which is blocked on #65202

[pietroalbini]

This seems to be finally fixed! Today nightly was shipped with #65274, and everything looks normal 🎉

[RalfJung]

Miri is "test-fail" now, so with the next nightly we should be able to see if indeed it gets removed from rustup as it should.

[pietroalbini]

Confirmed it's working! On nightly-2019-11-08 miri is missing.