Recent pre-nightly builds have "commit-hash: unknown" in their version info

[RalfJung]

I noticed this in Miri where we currently use the 668959740f97e7a22ae340742886d330ab63950f build (via rustup-toolchain-install-master):

$ rustc +miri --version -v
rustc 1.84.0-nightly
binary: rustc
commit-hash: unknown
commit-date: unknown
host: x86_64-unknown-linux-gnu
release: 1.84.0-nightly
LLVM version: 19.1.3

Something seems very wrong about this?

Cc @rust-lang/compiler (sorry I don't know whom specifically to ping here)

[RalfJung]

The latest nightly is still fine

$ rustc +nightly --version -v
rustc 1.84.0-nightly (59cec72a5 2024-11-08)
binary: rustc
commit-hash: 59cec72a57af178767a7b8e7f624b06cc50f1087
commit-date: 2024-11-08
host: x86_64-unknown-linux-gnu
release: 1.84.0-nightly
LLVM version: 19.1.3

[RalfJung]

That makes the regression range: 59cec72...6689597

[RalfJung]

Maybe 76b6090? Cc @onur-ozkan

[matthiaskrgr]

Is this miri toolchain an x.py install'd one?

The only situation where I would ever get a

commit-hash: unknown
commit-date: unknown

is when I x.py install something as a toolchain from a local branch and then try to get the --version of that .

[RalfJung]

Is this miri toolchain an x.py install'd one?

No, as I said: "via rustup-toolchain-install-master".

[matthiaskrgr]

👍 I did a quick bisect and its indeed #132772 ^^

[RalfJung]

So if we change nothing, the next nightly will also be like this.

Should we urgently revert #132772?

[jieyouxu]

I'll put up a revert for now, we can reland #132772.

[RalfJung]

You mean reland it later once we figured out the problem? Yes, sounds good.

[jieyouxu]

Yes

[RalfJung]

Okay, revert is in the queue. We should have sufficient time until the next nightly to get this landed in time.

I wonder if there's any way we can have a test for this... we do run a bunch of tests on the dist runners that cover the actually released artifacts, right?
Cc @rust-lang/infra

[jieyouxu]

It turns out we do have a related test: https://github.com/rust-lang/rust/blob/c24e166527cd46d3b4033b0c061d02657e3c3cbf/tests/run-make/version-verbose-commit-hash/rmake.rs

But that is conditioned on //@ needs-git-hash which is relying on bootstrap to report rustc was built with commit hash, and in this case the test never ran because unfortunately the bootstrap reports that commit hash was omitted.

(And thus it failed during the revert because rustc --version reports unknown)

[jieyouxu]

I wonder if there's any way we can have a test for this... we do run a bunch of tests on the dist runners that cover the actually released artifacts, right? Cc @rust-lang/infra

FWIW this reproduces even with ./x build library --stage 1

PS X:\repos\rust> rustc +stage1 -vV
rustc 1.84.0-dev
binary: rustc
commit-hash: unknown
commit-date: unknown
host: x86_64-pc-windows-msvc
release: 1.84.0-dev
LLVM version: 19.1.3

But there was only one run-make test that was guarded on git hash being enabled. Something simple like a shell script that runs after the dist artifact is built and double-checking the commit info is intact would be very nice.

[RalfJung]

Something simple like a shell script that runs after the dist artifact is built and double-checking the commit info is intact would be very nice.

That would also need a "needs-git-hash"-style gate though.

Seems more like the logic for needs-git-hash is at fault here? Could it be set so that on CI we always consider the compiler to have a git hash, no matter what the rest of the logic says?

[RalfJung]

I also don't understand why that test passed when the PR was landed but fails now with the revert...?

[jieyouxu]

Seems more like the logic for needs-git-hash is at fault here? Could it be set so that on CI we always consider the compiler to have a git hash, no matter what the rest of the logic says?

Yes. I would consider that problematic to only rely on bootstrap's info on whether the git commit info is expected to be enabled, it should have some kind of COMPILETEST_GIT_HASH=1 override for CI that would assert the test needs to run even if bootstrap incorrectly reports via --git-hash-enabled=false or whatever compiletest config flag. In that case, a CI post-dist script sanity check is probably still nice-to-have as a redundancy measure.

[RalfJung]

I think yet another test suite will be just mean more maintenance nightmare. We need to increase the reliability on our existing test suite, not duct-tape around it. It's clearly not very good to ask the system that we want to test whether it is working correctly; compiletest needs to independently check whether a git hash should be present.

But, why does this test fail now in the revert? A bunch of other PRs landed since the problematic one.

(In fact that PR landed before midnight, we didn't get a nightly last night... maybe due to this bug.)

[jieyouxu]

why that test passed when the PR was landed

When PR #132772 landed, the test could not have failed because //@ needs-git-hash is not satisfied (because in bootstrap we omitted the commit info), i.e. the test is straight up ignored.

but fails now with the revert...?

I'll need to double-check why... But I don't think the revert itself is wrong, it's just git revert -m1 rollup_commit_hash, something is very strange here...

[jieyouxu]

It's clearly not very good to ask the system that we want to test whether it is working correctly; compiletest needs to independently check whether a git hash should be present.

Agreed.

[RalfJung]

I'll need to double-check why... But I don't think the revert itself is wrong, it's just git revert -m1 rollup_commit_hash, something is very strange here...

Is it using a cached rustc rather than a freshly-built one?

As I already said in #131658, what we do there right now is quite fragile. So if that's what's causing the test failure here, that is further evidence supporting my case. If anything in src/bootstrap changes, we shouldn't use the cached rustc, as it's quite easy for some logic there to change some flags that change how rustc is built.

[onur-ozkan]

If anything in src/bootstrap changes, we shouldn't use the cached rustc, as it's quite easy for some logic there to change some flags that change how rustc is built.

Yeah, #131831 does that but it's still not landed :/

[jieyouxu]

Is it using a cached rustc rather than a freshly-built one?

It has to be doing something that like and using some kind of CI rustc, because the test failure is from commit hash being unknown and not hex digits (which should not be the case for a freshly built rustc with the PR reverted). Also test-various failed on stage 2 but like way too fast and could not have been freshly built when it failed.

[RalfJung]

Yeah, #131831 does that but it's still not landed :/

I see. Thanks for writing that PR. :)

IMO until that lands, we should not use cached rustc on CI -- that should be a safe PR that can land fairly quickly, I assume.
EDIT: like this: #132852

[onur-ozkan]

Closing since the issue is resolved.

[RalfJung]

Shouldn't we track somewhere that we had a test designed to catch this very mistake, and it didn't catch it?

[onur-ozkan]

Shouldn't we track somewhere that we had a test designed to catch this very mistake, and it didn't catch it?

I'm fine with keeping this open, but wouldn't it be better to create a specific issue for that necessary test and reference this issue from there?

[RalfJung]

Fair: #132875