search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.49k stars 12.73k forks source link

The run-make/version-verbose-commit-hash test doesn't reliably detect when the commit hash is missing #132875

Open RalfJung opened 2 days ago

RalfJung commented 2 days ago

We recently had pre-nightly builds without a commit hash in the verbose version info (https://github.com/rust-lang/rust/issues/132845). This is despite the fact that we have a test fully dedicated to preventing exactly that. Clearly, something is wrong with the test.

The issue with the test is that it has //@ needs-git-hash, which means it gets only run when bootstrap self-reports to compiletest that a git hash is available. This means if there is a bootstrap bug where a hash is never available, the test is skipped. Oopsie. Clearly, we shouldn't trust bootstrap on whether there should be a hash or not.

I don't know what exactly the conditions are under which a hash is legitimately missing. Probably it involves a build from the released tarball, or so?

I would suggest to remove the //@ needs-git-hash directive (it is only used by that one test), and instead have the test itself decide whether it can run or not. Maybe it can just check whether a .git folder exists? Maybe it can check whether it runs on CI, or runs on our CI? I don't know all the context here, but I hope it should be possible to independently determine whether the binary is supposed to have a hash.

Or maybe the simplest answer is to change this code to always pass --git-hash on CI. But it seems cleaner to me to not have to trust bootstrap when we are testing whether bootstrap did the right thing.

Cc @rust-lang/bootstrap

jieyouxu commented 2 days ago

I'm inclined to modify //@ needs-git-hash to check for a env var COMPILETEST_BUILT_WITH_GIT_HASH which is only set by CI instead of requesting and relying on such info as a bootstrap -> compiletest flag.