-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Omit checksum verification for local git dependencies #11188
Omit checksum verification for local git dependencies #11188
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) soon. Please see the contribution instructions for more information. |
☔ The latest upstream changes (presumably #11069) made this pull request unmergeable. Please resolve the merge conflicts. |
ae6e5d0
to
0408e9f
Compare
As I'm unlikely to get to this soon r? @weihanglo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look nicer! Thanks for the update.
let cksum = summary | ||
.checksum() | ||
.map(|s| s.to_string()) | ||
.filter(|s| !s.is_empty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we avoid this change? I feel like resolve graph should reflect what is inside the index, even it is an empty checksum = ""
. This also seems unnecessary in order to get cargo local-registry
works.
Also, it is a bit risky to change what is included in a resolver graph, as it would get serialized into lockfile, which we want to keep as stable as possible.
(Though I currently have yet found any case breaking the compatibility)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. The problem arises from the fact that the git dependency has no checksum, so when you generate a lockfile after adding a git dependency to a project, the relevant [[package]]
section does not contain a checksum field. When updating the dependencies, cargo local-registry
removes the source replacement configuration before running cargo::ops::resolve_ws
, which verifies the checksums. The way round this would be to do similar filtering in Resolve::merge_from
instead.
For example, after
cargo/src/cargo/core/resolver/resolve.rs
Line 158 in b690ab4
if let Some(mine) = self.checksums.get(id) { |
let mine = mine.as_ref().filter(|s| !s.is_empty());
let cksum = cksum.as_ref().filter(|s| !s.is_empty());
and this does appear to prevent the error from occurring.
re lockfile stability, my main expectation with the lockfile as a user is that enabling source replacement and running a build shouldn't cause the lockfile to change. The proposed alternative change does obviously cause the lockfile to change. My usual test for whether I have set up my local registry correctly is to verify cargo build --frozen
works and immediately after syncing the local registry, that will not work.
As far as I'm aware, only cargo local-registry
can generate empty checksums (as git dependencies, path dependencies and vendored dependencies all generate no checksum whatsoever) so there shouldn't be another case to break compatibility with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am now convinced. Thank you for explaining it again and again!
As far as I'm aware, only
cargo local-registry
can generate empty checksums (as git dependencies, path dependencies and vendored dependencies all generate no checksum whatsoever) so there shouldn't be another case to break compatibility with.
That's also what I observed. I am going to do an FCP and see if we can reach a consensus from the team.
@rfcbot merge This PR is primarily for local registry source. It contains two changes I'd like to call out:
I think these changes should be safe. From my observation, This is somewhat a special case for an external tool |
Team member @weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
Just wanted to note that there was a concern on how this would interact with updating a git dependency (whose package version doesn't change). Cargo's src cache may not know that the package has changed, and thus cargo may work incorrectly (using the stale cache). |
@rfcbot concern src-cache-not-update As @ehuss pointed out #11188 (comment), With the current patch, Cargo's src cache is not aware of a git dependency has been updated. Click to see steps to reproduce
If my findings are correct, the situation of this patch is still half broken. I am sorry I don't have too much inputs about how to fix it at this moment. |
I wanted to chime in on this from a slightly different angle given the context in #10071. My understanding of the ask here is that you want it to be possible to:
The challenge here, I think, is that Cargo has no way to know that a particular local registry entry is "really" a git dependency. We could add that kind of metadata, but if we do, nothing stops that same annotation to anything in the local registry, meaning we're really discussing whether it should be possible to opt-out of checksumming for any package in a local registry. For 1, the solution (I think) is to simply have Cargo allow the case where there is no checksum in For 2, I thought this case was already allowed since you're able to turn a For 3, this is the hard one. The approach taken in this PR runs into #10071 (which is ~= the concern currently raised), but more importantly it's going to mean that any entry in a local registry can be changed under Cargo, which is unlike how a "real" registry works. We (well, the Cargo team) will have to decide whether that's a feature they want Cargo local registries to have, even if the feature is partially obscured (such as having to add a "was a git dependency" marker). I have certainly run into cases where it'd be useful to be able to change something that's located in a local registry (and where I cannot use a directory registry instead), but I also acknowledge that it's a departure from the model local registries have traditionally had. |
@jarhodes314 I wanted to check in and see if you are still interested in working on this. It looks like there were some concerns raised about how cargo makes assumptions about checksums not changing, and those could cause problems. Is that something you can look at? |
☔ The latest upstream changes (presumably #13409) made this pull request unmergeable. Please resolve the merge conflicts. |
Going to cancel the FCP and close this. We're still interested in seeing a reasonable solution to the original issue. However, as Jon has outlined in #11188 (comment) it may not be as easy as we thought before. Thanks every for your participation. |
What does this PR try to resolve?
Fixes #10939
With the current implementation of the local registry source in cargo, all dependencies in a local registry require a valid checksum to be present before compilation. Git dependencies, however, don't have a checksum and therefore cannot pass this verification step, meaning git dependencies cannot be used from a local registry at present. This change skips the verification when a checksum is not present. In #10939, I have detailed why this change can only be effected within cargo itself, as opposed
to a change in cargo local-registry.
How should we test and review this PR?
I have created an example crate in https://github.com/jarhodes314/rust-lr-git-deps and explained in the README how this fails with the version of Cargo I have installed locally, and how it is fixed when instead using Cargo built from this branch.
Additional information
To maintain backwards compatibility withcargo local-registry
, all the checks are based on the checksum being an empty string for a git dependency, rather than null in the index. This mainly made testing my own changes easier. Since the git dependency functionality is entirely broken in existing versions ofcargo local-registry
, changing the index there to usenull
instead of""
in this case would be entirely possible if that is preferred from Cargo's side.Edit: In order to keep existing tests passing, I have changed the local-registry code to usenull
in the index rather than""
as it does currently for git dependencies. I have updated the demo repository accordingly.Further edit: Changing the local registry to use
null
requires a bunch of changes incargo
for relatively little benefit and also breaks backwards compatibility, so I reverted those changes and instead adjusted the failing test to provide a checksum.