Skip to content

Omit checksum verification for local git dependencies #11188

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

12 changes: 11 additions & 1 deletion crates/cargo-test-support/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@ pub struct Package {
alternative: bool,
invalid_json: bool,
proc_macro: bool,
generate_checksum: bool,
links: Option<String>,
rust_version: Option<String>,
cargo_features: Vec<String>,
Expand Down Expand Up @@ -860,6 +861,7 @@ impl Package {
alternative: false,
invalid_json: false,
proc_macro: false,
generate_checksum: true,
links: None,
rust_version: None,
cargo_features: Vec::new(),
Expand All @@ -877,6 +879,12 @@ impl Package {
self
}

/// Call with `true` to prevent a checksum being generated for the package.
pub fn skip_checksum(&mut self, local: bool) -> &mut Package {
self.generate_checksum = !local;
self
}

/// Call with `true` to publish in an "alternative registry".
///
/// The name of the alternative registry is called "alternative".
Expand Down Expand Up @@ -1075,9 +1083,11 @@ impl Package {
})
})
.collect::<Vec<_>>();
let cksum = {
let cksum = if self.generate_checksum {
let c = t!(fs::read(&self.archive_dst()));
cksum(&c)
} else {
String::new()
};
let name = if self.invalid_json {
serde_json::json!(1)
Expand Down
5 changes: 4 additions & 1 deletion src/cargo/core/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,10 @@ pub fn resolve(

let mut cksums = HashMap::new();
for (summary, _) in cx.activations.values() {
let cksum = summary.checksum().map(|s| s.to_string());
let cksum = summary
.checksum()
.map(|s| s.to_string())
.filter(|s| !s.is_empty());
Comment on lines +150 to +153
Copy link
Member

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)

Copy link
Author

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

if let Some(mine) = self.checksums.get(id) {
I added:

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.

Copy link
Member

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.

cksums.insert(summary.package_id(), cksum);
}
let graph = cx.graph();
Expand Down
6 changes: 5 additions & 1 deletion src/cargo/sources/registry/local.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,11 @@ impl<'cfg> RegistryData for LocalRegistry<'cfg> {
// We don't actually need to download anything per-se, we just need to
// verify the checksum matches the .crate file itself.
let actual = Sha256::new().update_file(&crate_file)?.finish_hex();
if actual != checksum {

// If a checksum of a package from local registry is empty,
// we assume it comes from a git dependency and can be skipped.
// (Probably be generated by tool `cargo-local-registry`)
if actual != checksum && !checksum.is_empty() {
anyhow::bail!("failed to verify the checksum of `{}`", pkg)
}

Expand Down
9 changes: 5 additions & 4 deletions tests/testsuite/directory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -410,10 +410,11 @@ fn crates_io_then_bad_checksum() {
p.cargo("build").run();
setup();

VendorPackage::new("bar")
.file("Cargo.toml", &basic_manifest("bar", "0.1.0"))
.file("src/lib.rs", "")
.build();
let mut v = VendorPackage::new("bar");
v.file("Cargo.toml", &basic_manifest("bar", "0.1.0"));
v.file("src/lib.rs", "");
v.cksum.package = Some("abcdef".into());
v.build();

p.cargo("build")
.with_status(101)
Expand Down
78 changes: 77 additions & 1 deletion tests/testsuite/local_registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

use cargo_test_support::paths::{self, CargoPathExt};
use cargo_test_support::registry::{registry_path, Package};
use cargo_test_support::{basic_manifest, project, t};
use cargo_test_support::{basic_manifest, git, path2url, project, t};
use std::fs;

fn setup() {
Expand Down Expand Up @@ -526,3 +526,79 @@ fn crates_io_registry_url_is_optional() {
p.cargo("build").with_stderr("[FINISHED] [..]").run();
p.cargo("test").run();
}

#[cargo_test]
fn git_dependencies_do_not_require_a_checksum() {
let git_project = git::new("dep1", |project| {
project
.file("Cargo.toml", &basic_manifest("bar", "0.0.1"))
.file("src/lib.rs", "pub fn bar() {}")
});

let p = project()
.file(
"Cargo.toml",
&format!(
r#"
[package]
name = "foo"
version = "0.0.1"
authors = []

[dependencies.bar]
git = '{}'
"#,
git_project.url()
),
)
.file(
"src/lib.rs",
"extern crate bar; pub fn foo() { bar::bar(); }",
)
.build();

p.cargo("generate-lockfile").run();

let root = paths::root();
t!(fs::create_dir(&root.join(".cargo")));

Package::new("bar", "0.0.1")
.local(true)
// Omit the checksum from the local registry by setting it to an empty string
// since git dependencies do not have checksums. Make sure that cargo doesn't
// treat this empty checksum as invalid.
// See https://github.com/rust-lang/cargo/pull/11188 for more info.
.skip_checksum(true)
.file("src/lib.rs", "pub fn bar() {}")
.publish();

t!(fs::write(
root.join(".cargo/config"),
format!(
r#"
[source.my-awesome-git-registry]
git = '{}'
replace-with = 'my-awesome-local-registry'

[source.my-awesome-local-registry]
local-registry = 'registry'
"#,
git_project.url()
)
));
p.cargo("clean").run();
p.cargo("build")
.with_stderr(&format!(
"[UNPACKING] bar v0.0.1 ([..])\n\
[COMPILING] bar v0.0.1 ({}#[..])\n\
[COMPILING] foo v0.0.1 ([CWD])\n\
[FINISHED] [..]\n",
path2url(&git_project.root())
))
.run();
p.cargo("build").with_stderr("[FINISHED] [..]").run();
p.cargo("test").run();
let lockfile = t!(fs::read_to_string(p.root().join("Cargo.lock")));
// We only have one dependency, and it should not contain a checksum in the lockfile
assert!(!lockfile.contains("checksum"));
}