fix: do not panic when failed to parse rustc commit-hash #12965

weihanglo · 2023-11-12T19:53:47Z

What does this PR try to resolve?

In some situation the commit-hash in rustc -vV output is "unknown".
The debug assertion must not block any progress on others like miri,

How should we test and review this PR?

See rust-lang/miri#3160

rustbot · 2023-11-12T19:53:53Z

r? @epage

(rustbot has picked a reviewer for you, use r? to override)

src/cargo/util/rustc.rs

epage · 2023-11-13T14:06:38Z

src/cargo/util/rustc.rs

+                    debug!("commit hash must be a hex string, got: {hash:?}");
+                    return false;
+                }
+                if hash.len() != 40 && hash.len() != 64 {


Do we have a way to verify that if rustc changes the commit hash format, that we'll update these checks?

I cannot think of a way now 😞.

We've already covered both SHA1 (40 chars) and SHA2 (64 chars) hash algorithms used by Cargo. I guess rustc won't change to any other format when Git is still in use. If people are going to add a new hash format into Git, it will be breaking news than this fails.

Normal developers also don't care about the hash here, since they always get a real rustc version, and the hash is only used by -Ztrim-paths now.

I cannot think of a way now 😞.

Do we have a test that the commit hash remapping works? Seems like that would cover this.

If I remember correctly, the assertion of hash part is omitted, as we don't know whether people run this with a rustc with or without git.

The alternative would be to specifically allow the hash to be "unknown", but still assert in all the other cases.

Or should we just remove these checks, since they are not really important?

The length checks? sure. Thats basically what Ralf suggested since our is_ascii_hexdigit check will filter out unknown.

Alternatively, we do if hash.len() < 40 to avoid short garbage (random words). Anything that looks like a hash that is longer is probably good enough.

Either way, I'm fine.

In some situation the commit-hash in `rustc -vV` output is "unknown". The debug assertion must not block any progress on others like miri.

src/cargo/util/rustc.rs

epage · 2023-11-13T21:09:21Z

@bors r+

bors · 2023-11-13T21:09:25Z

📌 Commit 7ca122c has been approved by epage

It is now in the queue for this repository.

bors · 2023-11-13T21:10:33Z

⌛ Testing commit 7ca122c with merge 4e02f11...

bors · 2023-11-13T21:53:51Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing 4e02f11 to master...

Update cargo 15 commits in 6790a5127895debec95c24aefaeb18e059270df3..87ee3e96285e0142b71d8c11c02b18647e43974d 2023-11-10 17:09:35 +0000 to 2023-11-14 18:00:46 +0000 - fix error message for duplicate links (rust-lang/cargo#12973) - Only filter out target if its in the package root (rust-lang/cargo#12944) - Ignore changing_spec_relearns_crate_types on windows-gnu (rust-lang/cargo#12972) - fix: do not panic when failed to parse rustc commit-hash (rust-lang/cargo#12965) - query{_vec} use IndexSummary (rust-lang/cargo#12970) - Bump to 0.77.0; update changelog (rust-lang/cargo#12966) - Improve about information of `cargo search` (rust-lang/cargo#12962) - Fix --quiet being used with nested subcommands. (rust-lang/cargo#12959) - make some debug assertion failures more informative (rust-lang/cargo#12963) - refactor(toml): Consistently lead with 'Toml' prefix (rust-lang/cargo#12960) - refactor(toml): Remove unused method (rust-lang/cargo#12961) - Fix non-deterministic behavior in last-use repopulation (rust-lang/cargo#12958) - Add cache garbage collection (rust-lang/cargo#12634) - refactor(toml): Improve consistency (rust-lang/cargo#12954) - Fix typo (rust-lang/cargo#12956) r? ghost

Update cargo 19 commits in 6790a5127895debec95c24aefaeb18e059270df3..2c03e0e2dcd05dd064fcf10cc1050d342eaf67e3 2023-11-10 17:09:35 +0000 to 2023-11-16 04:21:44 +0000 - docs(ref): Find a place to comment on --cap-lints (rust-lang/cargo#12976) - Switch from AtomicU64 to Mutex. (rust-lang/cargo#12981) - If the only path is a loop then counted as the shortest path. (rust-lang/cargo#12977) - fix(resolver): Prefer MSRV, rather than ignore incompatible (rust-lang/cargo#12950) - fix error message for duplicate links (rust-lang/cargo#12973) - Only filter out target if its in the package root (rust-lang/cargo#12944) - Ignore changing_spec_relearns_crate_types on windows-gnu (rust-lang/cargo#12972) - fix: do not panic when failed to parse rustc commit-hash (rust-lang/cargo#12965) - query{_vec} use IndexSummary (rust-lang/cargo#12970) - Bump to 0.77.0; update changelog (rust-lang/cargo#12966) - Improve about information of `cargo search` (rust-lang/cargo#12962) - Fix --quiet being used with nested subcommands. (rust-lang/cargo#12959) - make some debug assertion failures more informative (rust-lang/cargo#12963) - refactor(toml): Consistently lead with 'Toml' prefix (rust-lang/cargo#12960) - refactor(toml): Remove unused method (rust-lang/cargo#12961) - Fix non-deterministic behavior in last-use repopulation (rust-lang/cargo#12958) - Add cache garbage collection (rust-lang/cargo#12634) - refactor(toml): Improve consistency (rust-lang/cargo#12954) - Fix typo (rust-lang/cargo#12956)

rustbot assigned epage Nov 12, 2023

rustbot added A-cache-messages Area: caching of compiler messages S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 12, 2023

weihanglo mentioned this pull request Nov 12, 2023

cargo miri setup no longer works for locally installed toolchains rust-lang/miri#3160

Closed

RalfJung reviewed Nov 12, 2023

View reviewed changes

src/cargo/util/rustc.rs Outdated Show resolved Hide resolved

weihanglo force-pushed the hash branch from ba93b5c to 61cb6ee Compare November 13, 2023 02:29

epage reviewed Nov 13, 2023

View reviewed changes

fix: do not panic when failed to parse rustc commit-hash

7ca122c

In some situation the commit-hash in `rustc -vV` output is "unknown". The debug assertion must not block any progress on others like miri.

weihanglo force-pushed the hash branch from 61cb6ee to 7ca122c Compare November 13, 2023 20:48

epage reviewed Nov 13, 2023

View reviewed changes

src/cargo/util/rustc.rs Show resolved Hide resolved

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 13, 2023

bors merged commit 4e02f11 into rust-lang:master Nov 13, 2023
20 checks passed

weihanglo deleted the hash branch November 13, 2023 22:12

weihanglo mentioned this pull request Nov 14, 2023

Update cargo rust-lang/rust#117916

Merged

ehuss added this to the 1.76.0 milestone Dec 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: do not panic when failed to parse rustc commit-hash #12965

fix: do not panic when failed to parse rustc commit-hash #12965

weihanglo commented Nov 12, 2023 •

edited

rustbot commented Nov 12, 2023

epage Nov 13, 2023

weihanglo Nov 13, 2023

epage Nov 13, 2023

weihanglo Nov 13, 2023

RalfJung Nov 13, 2023

weihanglo Nov 13, 2023

epage Nov 13, 2023

epage commented Nov 13, 2023

bors commented Nov 13, 2023

bors commented Nov 13, 2023

bors commented Nov 13, 2023

fix: do not panic when failed to parse rustc commit-hash #12965

fix: do not panic when failed to parse rustc commit-hash #12965

Conversation

weihanglo commented Nov 12, 2023 • edited

What does this PR try to resolve?

How should we test and review this PR?

rustbot commented Nov 12, 2023

epage Nov 13, 2023

Choose a reason for hiding this comment

weihanglo Nov 13, 2023

Choose a reason for hiding this comment

epage Nov 13, 2023

Choose a reason for hiding this comment

weihanglo Nov 13, 2023

Choose a reason for hiding this comment

RalfJung Nov 13, 2023

Choose a reason for hiding this comment

weihanglo Nov 13, 2023

Choose a reason for hiding this comment

epage Nov 13, 2023

Choose a reason for hiding this comment

epage commented Nov 13, 2023

bors commented Nov 13, 2023

bors commented Nov 13, 2023

bors commented Nov 13, 2023

weihanglo commented Nov 12, 2023 •

edited