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

Cargo always updates the registry index with -Zbuild-std #8962

Closed
alexcrichton opened this issue Dec 9, 2020 · 7 comments
Closed

Cargo always updates the registry index with -Zbuild-std #8962

alexcrichton opened this issue Dec 9, 2020 · 7 comments
Labels
C-bug Category: bug

Comments

@alexcrichton
Copy link
Member

It looks like currently Cargo will always update the registry index when using -Zbuild-std:

$ cargo new foo
$ cd foo
$ cargo +nightly build -Zbuild-std --target ...
    Updating crates.io index
...

Bisection points to this being a regression from #8834, I'm currently poking around to see if I can find something.

@alexcrichton alexcrichton added the C-bug Category: bug label Dec 9, 2020
@ehuss
Copy link
Contributor

ehuss commented Dec 10, 2020

☹️ Oh, that's a tricky one. I think the issue is that the original Cargo.lock has rustc-workspace-std-* packages registered as local path packages (because the lock was built with those in the [patch] table), but the vendored dependencies (like cfg-if) are registered in the lock as crates.io packages. By adding cfg-if to the patch set, cfg-if ends up with an unlocked dependency for rustc-workspace-std-core (because cfg-if is expecting that dependency to be from crates.io), which triggers the update.

I can't think of a simple way to fix that. Vendoring should probably use source replacement (not patches), but source replacement is a global setting that can't be easily enabled just for resolving std.

@jgouly
Copy link

jgouly commented Dec 10, 2020

I think this also stops you from using --offline with build-std.

@alexcrichton
Copy link
Member Author

Hm so this is one possible solution:

diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs
index 0380c447d..38f00d045 100644
--- a/src/cargo/core/registry.rs
+++ b/src/cargo/core/registry.rs
@@ -726,7 +726,9 @@ fn lock(
 
         // If this dependency did not have a locked version, then we query
         // all known locked packages to see if they match this dependency.
-        // If anything does then we lock it to that and move on.
+        // If anything does then we lock it to that and move on. This will help
+        // us to use an existing lock file as much as possible, even when new
+        // nodes and dependencies are introduced.
         let v = locked
             .get(&(dep.source_id(), dep.package_name()))
             .and_then(|vec| vec.iter().find(|&&(id, _)| dep.matches_id(id)));
@@ -737,6 +739,25 @@ fn lock(
             return dep;
         }
 
+        // Ok and failing all that, we make one last ditch attempt to search the
+        // patches as well. If our dependency matches a patch and that patch is
+        // additionally locked already, then we unconditionally use the locked
+        // patched dependency's version.
+        let v = patches
+            .get(dep.source_id().canonical_url())
+            .and_then(|vec| vec.iter().find(|id| dep.matches_ignoring_source(**id)));
+        if let Some(id) = v {
+            if let Some(locked_list) = locked.get(&(id.source_id(), id.name())) {
+                if locked_list.iter().any(|(a, _)| id == a) {
+                    trace!("\tthird hit on {}", id);
+                    let mut dep = dep;
+                    let req = VersionReq::exact(id.version());
+                    dep.set_version_req(req);
+                    return dep;
+                }
+            }
+        }
+
         trace!("\tnope, unlocked");
         dep
     })
diff --git a/tests/testsuite/patch.rs b/tests/testsuite/patch.rs
index 8e2a3b1a3..20a82b32d 100644
--- a/tests/testsuite/patch.rs
+++ b/tests/testsuite/patch.rs
@@ -2012,3 +2012,66 @@ fn can_update_with_alt_reg() {
         )
         .run();
 }
+
+#[cargo_test]
+fn use_for_new_registry_dependencies() {
+    Package::new("dep1", "0.1.0").dep("bar", "0.1").publish();
+    Package::new("bar", "0.1.0").publish();
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                 [package]
+                 name = "foo"
+                 version = "0.1.0"
+
+                 [dependencies]
+                 dep1 = "0.1"
+
+                 [patch.crates-io]
+                 bar = { path = "bar" }
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .file("bar/Cargo.toml", &basic_manifest("bar", "0.1.0"))
+        .file("bar/src/lib.rs", "")
+        .file(
+            "dep2/Cargo.toml",
+            r#"
+                 [package]
+                 name = "dep2"
+                 version = "0.1.0"
+
+                 [dependencies]
+                 bar = "0.1"
+            "#,
+        )
+        .file("dep2/src/lib.rs", "")
+        .build();
+
+    p.cargo("check").run();
+    p.change_file(
+        "Cargo.toml",
+        r#"
+             [package]
+             name = "foo"
+             version = "0.1.0"
+
+             [dependencies]
+             dep1 = "0.1"
+             dep2 = { path = 'dep2' }
+
+             [patch.crates-io]
+             bar = { path = "bar" }
+        "#,
+    );
+    p.cargo("check")
+        .with_stderr(
+            "\
+[CHECKING] dep2 [..]
+[CHECKING] foo [..]
+[FINISHED] [..]
+",
+        )
+        .run();
+}

This is arguably a preexisting bug where if you add a dependency which transitively depends on something already patched it updates the index unnecessarily. I'm not overly confident in this diff though, it feels a bit too situation-specific and possibly runs afoul of making a too-constrained dependency graph.

@alexcrichton
Copy link
Member Author

Hm with this and #8963 should we consider backing out #8834? This is probably more breakage than expected...

@ehuss
Copy link
Contributor

ehuss commented Dec 10, 2020

There is also rust-lang/rust#79218. Unless @Gankra wants to put significant effort to fix the issues before the next beta, I think it might be a good idea to revert that and rust-lang/rust#78790. I am unlikely to put in the effort over the next 3 weeks to fix those issues.

@Mark-Simulacrum
Copy link
Member

Yeah I think reverting is entirely reasonable, we can always reland

bors added a commit that referenced this issue Dec 12, 2020
Revert recent build-std vendoring changes

This reverts #8834 to address #8962 and #8963

cc `@Gankra`
@alexcrichton
Copy link
Member Author

Fixed with #8968 being merged

JohnTitor added a commit to JohnTitor/rust that referenced this issue Dec 17, 2020
…k-Simulacrum

Revert rust-lang#78790 - rust-src vendoring

This reverts the rust-src vendor changes from rust-lang#78790. There were a few issues (see rust-lang#79218, rust-lang/cargo#8962, rust-lang/cargo#8963), that I don't think will get fixed in the next few days before the beta branch.

Fixes rust-lang#79218
bors added a commit that referenced this issue Apr 23, 2021
Fix build-std updating the index on every build.

rust-lang/rust#83776 has caused a problem where build-std will update the index on every build. That PR added `std_detect` from the `stdarch` submodule as a path dependency of `std`. However, since `stdarch` has a workspace of its own, an exclusion had to be added to `Cargo.toml` so that it does not treat `std_detect` as a workspace member (because nested workspaces are not supported).

The problem is that the std `Cargo.lock` file is built thinking that `std_detect` is *not* a workspace member. This means that its dev-dependencies are not included. However, when cargo resolves the std workspace, it doesn't know that `std_detect` should be excluded, so it considers it a workspace member (because it is a path dependency). This means that it expects the dev-dependencies to be in Cargo.lock. Because they are missing, it ends up poisoning the registry and triggering an update:

> poisoning registry `https://github.com/rust-lang/crates.io-index` because std_detect v0.1.5 (/Users/eric/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/stdarch/crates/std_detect) looks like it changed auxv

The solution here is to skip dev-dependencies if they are not actively being resolved, even if the package is a workspace member.

This has happened before (#8962), so I have updated the test to check for it.

There are some alternative solutions I considered:

* Add support for nested workspaces. 😄
* Use a symlink to `std_detect` in the `rust-lang/rust` repository so that it appears to cargo as-if it is "outside" of the stdarch workspace, and thus can be treated like a normal workspace member (and remove the "exclude"). That seems a little hacky.

Fixes #9390
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

No branches or pull requests

4 participants