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

Add unstable support for outputting file checksums for use in cargo #126930

Merged
merged 12 commits into from
Oct 3, 2024

Conversation

Xaeroxe
Copy link
Contributor

@Xaeroxe Xaeroxe commented Jun 25, 2024

Adds an unstable option that appends file checksums and expected lengths to the end of the dep-info file such that cargo can read and use these values as an alternative to file mtimes.

This PR powers the changes made in this cargo PR rust-lang/cargo#14137

Here's the tracking issue for the cargo feature rust-lang/cargo#14136.

@rustbot
Copy link
Collaborator

rustbot commented Jun 25, 2024

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 25, 2024
@Urgau
Copy link
Member

Urgau commented Jun 25, 2024

Given that this PR adds a new concept and a new flag it requires a minimum of discussion within the team.
Could you start a T-compiler Major Change Proposol following the guide.

@rustbot
Copy link
Collaborator

rustbot commented Jun 26, 2024

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Jul 1, 2024

This now has an MCP rust-lang/compiler-team#765

@fmease
Copy link
Member

fmease commented Jul 7, 2024

Waiting for rust-lang/compiler-team#765 FCP to complete. That is 2024-07-11 07:23h UTC.
@rustbot blocked

@rustbot rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 7, 2024
@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Jul 13, 2024

The MCP has been accepted

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jul 13, 2024
@rustbot rustbot 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) labels Jul 13, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jul 13, 2024

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Jul 13, 2024

Correct, blake3 has been added as it was mentioned during the MCP review as being desirable.

@bors

This comment was marked as outdated.

@Urgau
Copy link
Member

Urgau commented Jul 17, 2024

We had some LLVM BOLT issues with the blake3 crate in #127754, a couple of days ago, on a simple try build.
Let's try this PR, to see if is also has those compilation issues.

@bors try (@Xaeroxe could you fix the conflicts please)

@bors

This comment was marked as outdated.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Jul 17, 2024

@Urgau conflicts resolved.

@Urgau
Copy link
Member

Urgau commented Jul 17, 2024

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 17, 2024
@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Oct 2, 2024

Follow-up at #131139

@workingjubilee
Copy link
Member

The rollup hasn't actually started, so it's actually quite cost-free to just

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 2, 2024
@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Oct 2, 2024

Okay, in that case here is the commit that @weihanglo wanted b48c5f1

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Oct 2, 2024

The Cargo PR has already been adjusted to this new behavior.

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked that 58c5ac4 works with the latest commit rust-lang/cargo@c755949 in rust-lang/cargo#14137.

I would like to r+ this, but I am not in t-compiler so probably shouldn't do that.

It would be appreciated if @chenyukang or someone on t-compiler could take another look. It should be pretty much the same as before except writing # checksum comments to separate lines (b48c5f1)

@chenyukang
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 3, 2024

📌 Commit 58c5ac4 has been approved by chenyukang

It is now in the queue for this repository.

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 3, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 3, 2024
…kingjubilee

Rollup of 3 pull requests

Successful merges:

 - rust-lang#126930 (Add unstable support for outputting file checksums for use in cargo)
 - rust-lang#130725 (Parser: better error messages for ``@`` in struct patterns)
 - rust-lang#131166 (Fix `target_vendor` for `aarch64-nintendo-switch-freestanding`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 44f6275 into rust-lang:master Oct 3, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Oct 3, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 3, 2024
Rollup merge of rust-lang#126930 - Xaeroxe:file-checksum-hint, r=chenyukang

Add unstable support for outputting file checksums for use in cargo

Adds an unstable option that appends file checksums and expected lengths to the end of the dep-info file such that `cargo` can read and use these values as an alternative to file mtimes.

This PR powers the changes made in this cargo PR rust-lang/cargo#14137

Here's the tracking issue for the cargo feature rust-lang/cargo#14136.
@klensy
Copy link
Contributor

klensy commented Oct 3, 2024

Can PR description be updated with actual changes in that PR? Currently is slightly misleading.

Comment on lines +1706 to +1707
checksum_hash_algorithm: Option<SourceFileHashAlgorithm> = (None, parse_cargo_src_file_hash, [TRACKED],
"hash algorithm of source files used to check freshness in cargo (`blake3` or `sha256`)"),
Copy link
Contributor

@klensy klensy Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happened if md5 passed here? SourceFileHashAlgorithm allows it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then md5 checksums will be emitted. This doesn’t make any guarantees about whether the tooling surrounding rustc can actually ingest those. It’s up to the tooling to understand what it can ingest. An earlier version of this PR did forbid things like md5 but I was instructed to not build in that kind of restriction by prior reviewers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, at least help message mentions only blake3 and sha256, while actually accepting something else too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm… still seeing "cargo" mentioned here and elsewhere. Not really critical but would love to see we make the option more general than a Cargo-specific compiler flag (though in fact it was made for Cargo 😆)

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Oct 3, 2024

Can PR description be updated with actual changes in that PR? Currently is slightly misleading.

How so? I just went back and read it and it seems correct to me. What’s misleading about it?

@Xaeroxe Xaeroxe deleted the file-checksum-hint branch October 3, 2024 12:41
@klensy
Copy link
Contributor

klensy commented Oct 3, 2024

PR also adds option for using blake3 source file hashing (or not? I'm not clearly understand that, the same enum used for source file hash for debug and for new cargo feature)

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Oct 3, 2024

blake3 can’t be used in debug data as no debug format supports it, but it can be used in dep-info files. If you try to use blake3 here then you simply won’t get a hash in your debug files.

bors added a commit to rust-lang/cargo that referenced this pull request Oct 4, 2024
initial version of checksum based freshness

Implementation for #14136 and resolves #6529

This PR implements the use of checksums in cargo fingerprints as an alternative to using mtimes. This is most useful on systems with poor mtime implementations.

This has a dependency on rust-lang/rust#126930. It's expected this will increase the time it takes to declare a build to be fresh. Still this loss in performance may be preferable to the issues the ecosystem has had with the use of mtimes for determining freshness.
bors added a commit to rust-lang/cargo that referenced this pull request Oct 8, 2024
initial version of checksum based freshness

Implementation for #14136 and resolves #6529

This PR implements the use of checksums in cargo fingerprints as an alternative to using mtimes. This is most useful on systems with poor mtime implementations.

This has a dependency on rust-lang/rust#126930. It's expected this will increase the time it takes to declare a build to be fresh. Still this loss in performance may be preferable to the issues the ecosystem has had with the use of mtimes for determining freshness.
@weihanglo weihanglo mentioned this pull request Oct 9, 2024
bors pushed a commit to rust-lang-ci/rust that referenced this pull request Oct 9, 2024
This also adds three license exceptions to Cargo.

* arrayref — BSD-2-Clause
* blake3 — CC0-1.0 OR Apache-2.0 OR Apache-2.0 WITH LLVM-exception
* constant_time_eq — CC0-1.0 OR MIT-0 OR Apache-2.0

These exceptions were added to rustc in rust-lang#126930,
so should be fine for Cargo as well.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 9, 2024
Update cargo

8 commits in ad074abe3a18ce8444c06f962ceecfd056acfc73..15fbd2f607d4defc87053b8b76bf5038f2483cf4
2024-10-04 18:18:15 +0000 to 2024-10-08 21:08:11 +0000
- initial version of checksum based freshness (rust-lang/cargo#14137)
- feat: Add custom completer for completing registry name (rust-lang/cargo#14656)
- Document build-plan as being deprecated (rust-lang/cargo#14657)
- fix(complete): Don't complete files for any value (rust-lang/cargo#14653)
- Add more SAT resolver tests (rust-lang/cargo#14614)
- fix: avoid inserting duplicate `dylib_path_envvar` when calling `cargo run` recursively (rust-lang/cargo#14464)
- chore(deps): bump gix-path from 0.10.9 to 0.10.11 (rust-lang/cargo#14489)
- improve error reporting when feature not found in `activated_features` (rust-lang/cargo#14647)

---

This also adds three license exceptions to Cargo.

* arrayref — BSD-2-Clause
* blake3 — CC0-1.0 OR Apache-2.0 OR Apache-2.0 WITH LLVM-exception
* constant_time_eq — CC0-1.0 OR MIT-0 OR Apache-2.0

These exceptions were added to rustc in rust-lang#126930, so should be fine for Cargo as well.
rust-cloud-vms bot pushed a commit to liwagu/rust that referenced this pull request Oct 10, 2024
This also adds three license exceptions to Cargo.

* arrayref — BSD-2-Clause
* blake3 — CC0-1.0 OR Apache-2.0 OR Apache-2.0 WITH LLVM-exception
* constant_time_eq — CC0-1.0 OR MIT-0 OR Apache-2.0

These exceptions were added to rustc in rust-lang#126930,
so should be fine for Cargo as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.