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

don't write a an incorrect rustc version to the fingerprint file #6473

Merged
merged 1 commit into from
Dec 21, 2018

Conversation

Eh2406
Copy link
Contributor

@Eh2406 Eh2406 commented Dec 21, 2018

In making holmgr/cargo-sweep#14 I noted that some fingerprint files related to build scripts report being built with a rustc that hashes to "0". To work around this I just marked them as still being needed, even though no rustc that hashes to "0" is currently installed.

I believe that this PR just filles in the correct info for the build script fingerprints. This makes it possible for outside tools to more reliably clean up after cargo, at basically no cost.
@ehuss Thanks again for the help.

@rust-highfive
Copy link

r? @nrc

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

@alexcrichton
Copy link
Member

This is actually somewhat intentional in the sense that the fingerprint file here is for a build script's execution rather than the binary itself. That means that the rustc binary isn't actually an input to the execution itself?

@Eh2406
Copy link
Contributor Author

Eh2406 commented Dec 21, 2018

I believe that is technically true. From my experimentation (please correct me if I am wrong) there is no way to reuse the files between invocations involving difrentvertions of rust. I think this is because the hash in the folder name includes the version of rust, and even if that were fixed the hash includes the hash of the dependencies fingerprints which include the rust version. So if (I am correct and) there is in practice no reuse between different versions of rust, then we may as well make it clear.

@alexcrichton
Copy link
Member

That sounds right to me yeah! In some cases we may too aggrressively include the upstream hashes too...

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Dec 21, 2018

📌 Commit eb8720a has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Dec 21, 2018

⌛ Testing commit eb8720a with merge b61f3cc...

bors added a commit that referenced this pull request Dec 21, 2018
don't write a an incorrect rustc version to the fingerprint file

In making holmgr/cargo-sweep#14 I noted that some fingerprint files related to build scripts report being built with a rustc that hashes to "0". To work around this I just marked them as still being needed, even though no rustc that hashes to "0" is currently installed.

I believe that this PR just filles in the correct info for the build script fingerprints. This makes it possible for outside tools to more reliably clean up after cargo, at basically no cost.
@ehuss Thanks again for the help.
@Eh2406
Copy link
Contributor Author

Eh2406 commented Dec 21, 2018

So just to clarify Cargo does not need this. Cargo uses the full hash in the filename to prevent reuse. The full hash is basically impossible to reverse engineer, it is only useful to the version of cargo that made the file. This is basically the same kind of reason why we have a "fingerprint.json" file, so that we can do a fast compare by hash value, but look at the "fingerprint.json" to find out what changed.

My PR to cargo-sweep {ab}uses the "fingerprint.json" file to determine which version of rust is hidden in the full hash. Then uses that information to determine if it is safe to dell the corresponding artifacts. This is a somewhat elegant hack to allow experimentation with a target folder GC as a third party subcommand.

This PR does not change the amount of reuse cargo can have (the rust version is transitively included in the full hash), but does make that explicit so that the hack in cargo-sweep can clean up the files.

@bors
Copy link
Contributor

bors commented Dec 21, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing b61f3cc to master...

@bors bors merged commit eb8720a into rust-lang:master Dec 21, 2018
@Eh2406 Eh2406 deleted the rustc-0 branch December 22, 2018 18:00
bors added a commit to rust-lang/rust that referenced this pull request Jan 4, 2019
Update cargo

24 commits in 0d1f1bbeabd5b43a7f3ecfa16540af8e76d5efb4..34320d212dca8cd27d06ce93c16c6151f46fcf2e
2018-12-19 14:45:14 +0000 to 2019-01-03 19:12:38 +0000
- Display environment variables for rustc commands (rust-lang/cargo#6492)
- Fix a very minor race condition in `cargo fix`. (rust-lang/cargo#6515)
- Add a high-level overview of how `fix` works. (rust-lang/cargo#6516)
- Add dependency `registry` to `cargo metadata`. (rust-lang/cargo#6500)
- Fix fingerprint calculation for patched deps. (rust-lang/cargo#6493)
- serialize version directly (rust-lang/cargo#6512)
- use DYLD_FALLBACK_LIBRARY_PATH for dylib_path_envvar on macOS (rust-lang/cargo#6355)
- Fix error message when resolving dependencies (rust-lang/cargo#6510)
- use PathBuf in cargo metadata (rust-lang/cargo#6511)
- Fixed link to testsuite in CONTRIBUTING.md (rust-lang/cargo#6506)
- Update display of contents of Cargo.toml (rust-lang/cargo#6501)
- Update display of contents of Cargo.toml (rust-lang/cargo#6502)
- Fixup cargo install's help message (rust-lang/cargo#6495)
- testsuite: Require failing commands to check output. (rust-lang/cargo#6497)
- Delete unnecessary 'return' (rust-lang/cargo#6496)
- Fix new unused patch warning. (rust-lang/cargo#6494)
- Some minor documentation changes. (rust-lang/cargo#6481)
- Add `links` to `cargo metadata`. (rust-lang/cargo#6480)
- Salvaged semver work (rust-lang/cargo#6476)
- Warn on unused patches. (rust-lang/cargo#6470)
- don't write a an incorrect rustc version to the fingerprint file (rust-lang/cargo#6473)
- Rewrite `login` and registry cleanups. (rust-lang/cargo#6466)
- [issue#6461] Fix cargo commands list (rust-lang/cargo#6462)
- Restrict registry names to same style as package names. (rust-lang/cargo#6469)
@ehuss ehuss added this to the 1.33.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants