Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
RalfJung opened this issue Nov 10, 2024 · 1 comment
Assignees
Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@RalfJung
Copy link
Member

RalfJung commented Nov 10, 2024

We recently had pre-nightly builds without a commit hash in the verbose version info (#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

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 10, 2024
@onur-ozkan onur-ozkan added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-run-make Area: port run-make Makefiles to rmake.rs and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 10, 2024
@jieyouxu jieyouxu added the C-bug Category: This is a bug. label Nov 11, 2024
@jieyouxu
Copy link
Member

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.

@jieyouxu jieyouxu self-assigned this Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
Development

No branches or pull requests

4 participants