Support dependencies from registries for artifact dependencies, take 2 #12421

fee1-dead · 2023-07-31T14:11:49Z

This is a continuation of #12062, and closes #10405.

Here are things this PR changes:

Add information about artifact dependencies to the index. This bumps index_v to 3 for people using bindeps.
Make RustcTargetData mutable for the feature resolver. This is so that we can query rustc for target info as late as resolving features, as that is when we really know if a package is really going to be used.

bors · 2023-08-01T15:24:48Z

☔ The latest upstream changes (presumably #12426) made this pull request unmergeable. Please resolve the merge conflicts.

ehuss · 2023-08-15T19:31:13Z

@fee1-dead Just wanted to check in and acknowledge that I have seen this PR, but I haven't had the time to look at it. I will try to get to it soon.

ehuss · 2023-08-18T22:55:47Z

src/cargo/sources/registry/index.rs

@@ -178,7 +178,7 @@ struct Summaries {
 /// A lazily parsed [`IndexSummary`].
 enum MaybeIndexSummary {
    /// A summary which has not been parsed, The `start` and `end` are pointers
-    /// into [`Summaries::raw_data`] which this is an entry of.
+    /// into [`Summaries::raw_data`] which this isRegistryDependency an entry of.


This looks like a stray edit?

Suggested change

/// into [`Summaries::raw_data`] which this isRegistryDependency an entry of.

/// into [`Summaries::raw_data`] which this is an entry of.

ehuss · 2023-08-18T23:10:27Z

tests/testsuite/artifact_dep.rs

 #[cargo_test]
-#[ignore = "broken, need artifact info in index"]


In targets_are_picked_up_from_non_workspace_artifact_deps, in the part that says Package::new("uses-artifact", "1.0.0"), can you add .schema_version(3) to make sure the index filtering is working correctly?

ehuss · 2023-08-18T23:18:03Z

src/cargo/core/resolver/features.rs

+                                // and per-pkg-targets are not immediately known.
+                                let mut activate_target = |target| {
+                                    self.target_data
+                                        .merge_compile_kind(CompileKind::Target(target))


If this fails, it seems to generate a confusing error. See check_transitive_artifact_dependency_with_different_target. I think what would help is to add .with_context(|| format!("...")) with a message that says something like "failed to determine target information for target {target} for artifact dependency {name} in package {pkg_id}".

ehuss · 2023-08-18T23:18:21Z

tests/testsuite/artifact_dep.rs

@@ -2894,8 +2896,7 @@ fn check_transitive_artifact_dependency_with_different_target() {
    p.cargo("check -Z bindeps")
        .masquerade_as_nightly_cargo(&["bindeps"])
        .with_stderr_contains(
-            "error: could not find specification for target `custom-target`.\n  \
-            Dependency `baz v0.0.0 [..]` requires to build for target `custom-target`.",
+            "error: failed to run `rustc` to learn about target-specific information",


This error message seems pretty vague. I added a comment above to enhance in activate_target.

ehuss · 2023-08-19T00:45:15Z

src/cargo/sources/registry/index.rs

@@ -807,7 +815,7 @@ impl<'a> SummariesCache<'a> {
            .get(..4)
            .ok_or_else(|| anyhow::anyhow!("cache expected 4 bytes for index schema version"))?;
        let index_v = u32::from_le_bytes(index_v_bytes.try_into().unwrap());
-        if index_v != INDEX_V_MAX {
+        if index_v != INDEX_V_MAX && index_v != 3 {


Can you say more about why this check was added? It doesn't look like v3 is being written in SummariesCache::serialize anyway, so it is always 2.

I'm thinking this isn't necessary (yet), because older cargos don't fail to parse the new entries. The key part of this index check is IndexSummary::parse. If we make a change that causes that to fail, then older cargos will ignore those entries, and not place them in the cache (it assumes they are from a future format it does not understand). However, since bindeps is just adding new fields to the JSON, that doesn't cause a problem (new fields are ignored).

When we stabilize, I think we'll need to set INDEX_V_MAX to 3, but I'm not sure it is necessary to make any changes at this time.

I think this was simply carried over from the last PR. I don't remember my reasoning for this and I have removed this condition.

ehuss · 2023-08-19T00:50:22Z

Thanks, this looks great!

Can you add a test to make sure the v index version filtering works? I think that would look like something like this:

Package::new("bar", "1.0.0").publish();
Package::new("bar", "1.0.1")
    .schema_version(3)
    .add_dep(dep.artifact("bin", Some(target.to_string())))
    .publish();

// Verify that without `-Zbindeps` that it does not use 1.0.1.
let p = project()
    .file("Cargo.toml",
        r#"
            [package]
            name = "foo"
            version = "0.1.0"

            [dependency]
            bar = "1.0"
        "#)
    .file("src/lib.rs", "")
    .build();

p.cargo("tree").with_stdout("...").run();

// And with -Zbindeps it can use 1.0.1.
p.cargo("update -Zbindeps").with_stderr("...").run();

// And without -Zbindeps, now that 1.0.1 is in Cargo.lock, it should fail.
p.cargo("check").with_status(101).with_stderr("...").run();

I think that should roughly cover it, but if you have other ideas on how to try to break it, I think it would be good to make sure that has plenty of coverage.

I noticed that this doesn't update download_accessible. Were there problems with that, or is that something that we can iterate on later? (Not a blocker for this PR.)

Related to download_accessible, I think FeatureResolver::is_proc_macro will panic if it needs to process a proc-macro loaded from an artifact dependency with a target, and there is some problem downloading (like a network error). That is, something like the following will panic:

#[cargo_test]
fn proc_macro_in_artifact_dep() {
    // Forcing FeatureResolver to check a proc-macro for a dependency behind a
    // target dependency.
    if cross_compile::disabled() {
        return;
    }
    Package::new("pm", "1.0.0")
        .file("src/lib.rs", "")
        .file(
            "Cargo.toml",
            r#"
                [package]
                name = "pm"
                version = "1.0.0"

                [lib]
                proc-macro = true

            "#,
        )
        .publish();
    let alternate = cross_compile::alternate();
    Package::new("bin-uses-pm", "1.0.0")
        .target_dep("pm", "1.0", alternate)
        .file("src/main.rs", "fn main() {}")
        .publish();
    // Simulate a network error downloading the proc-macro.
    std::fs::remove_file(cargo_test_support::paths::root().join("dl/pm/1.0.0/download")).unwrap();
    let p = project()
        .file(
            "Cargo.toml",
            &format!(
                r#"
                [package]
                name = "foo"
                version = "0.1.0"
                edition = "2021"

                [dependencies]
                bin-uses-pm = {{ version = "1.0", artifact = "bin", target = "{alternate}"}}
            "#
            ),
        )
        .file("src/lib.rs", "")
        .build();

    p.cargo("check -Z bindeps")
        .masquerade_as_nightly_cargo(&["bindeps"])
        .with_stderr("")
        .run();
}

Can you make sure to document IndexPackage::v with the new version 3 changes?

fee1-dead · 2023-08-19T04:54:08Z

Can you add a test to make sure the v index version filtering works?

added

I noticed that this doesn't update download_accessible. Were there problems with that, or is that something that we can iterate on later? (Not a blocker for this PR.)

Yeah, let's iterate on this later, I specifically did not look into updating download_accessible because it turned out to be a bit complicated for the last PR. I've added your test case as an ignored test.

Can you make sure to document IndexPackage::v with the new version 3 changes?

Yep, done.

fee1-dead · 2023-08-19T05:43:01Z

not sure why check-version-bump is failing for cargo-credential. This PR doesn't modify that crate

bors · 2023-08-24T11:58:06Z

☔ The latest upstream changes (presumably #12381) made this pull request unmergeable. Please resolve the merge conflicts.

ehuss · 2023-08-24T20:15:19Z

not sure why check-version-bump is failing for cargo-credential. This PR doesn't modify that crate

I think there was an issue that was since fixed (#12508).

ehuss · 2023-08-24T20:44:38Z

Thanks, looks good! I appreciate you working on this.

I filed #12554 for the download_accessible issue, and #12555 to track updating crates.io (though I'm not sure when we should actually do that).

Can you resolve the merge conflict? Unfortunately it looks like I don't have permission to push to your branch.

ehuss · 2023-08-25T04:08:44Z

Thanks!

@bors r+

bors · 2023-08-25T04:08:46Z

📌 Commit 43dccc7 has been approved by ehuss

It is now in the queue for this repository.

bors · 2023-08-25T04:08:53Z

⌛ Testing commit 43dccc7 with merge e2c6a3e...

bors · 2023-08-25T04:50:31Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing e2c6a3e to master...

Update cargo 13 commits in 2cc50bc0b63ad20da193e002ba11d391af0104b7..925280f028db3a322935e040719a0754703947cf 2023-08-22 22:43:08 +0000 to 2023-08-25 21:16:44 +0000 - string leek is stable (rust-lang/cargo#12559) - refactor: Pull out cargo-add MSRV code for reuse (rust-lang/cargo#12553) - Contrib: Add process for security responses. (rust-lang/cargo#12487) - Support dependencies from registries for artifact dependencies, take 2 (rust-lang/cargo#12421) - fix(toml): Improve parse errors (rust-lang/cargo#12556) - Create dedicated unstable flag for asymmetric-token (rust-lang/cargo#12551) - chore(deps): update latest msrv to v1.72.0 (rust-lang/cargo#12549) - changelog: add link to CVE-2023-40030 (rust-lang/cargo#12550) - refactor(install): Move value parsing to clap (rust-lang/cargo#12547) - fix: Set MSRV for internal packages (rust-lang/cargo#12381) - doc: fix two links to tracing docs (rust-lang/cargo#12537) - use AND search when having multiple terms (rust-lang/cargo#12548) - fix(log): Use a more compact relative-time format (rust-lang/cargo#12542) r? ghost

elchukc · 2024-10-23T22:00:26Z

I think FeatureResolver::is_proc_macro will panic if it needs to process a proc-macro loaded from an artifact dependency with a target, and there is some problem downloading (like a network error).

@ehuss Working on a fix for this test right now. Is this an example of a spurious network error, for which the desired behaviour that it retries?

ehuss · 2024-10-25T14:07:13Z

@elchukc Sorry, I'm not quite understanding your question. There is a test (already checked in) called proc_macro_in_artifact_dep. It demonstrates what happens if there is any kind of network error when downloading a proc-macro (it currently panics). The fix should make it so that doesn't panic, but instead provide a reasonable error message.

rustbot assigned ehuss Jul 31, 2023

fee1-dead marked this pull request as draft July 31, 2023 14:31

fee1-dead force-pushed the bindeps-registry2 branch from 0c0e79c to 5be6864 Compare July 31, 2023 14:41

fee1-dead marked this pull request as ready for review July 31, 2023 17:31

fee1-dead force-pushed the bindeps-registry2 branch from 5be6864 to f55f4c5 Compare August 7, 2023 09:18

ehuss reviewed Aug 19, 2023

View reviewed changes

fee1-dead force-pushed the bindeps-registry2 branch 2 times, most recently from df276af to 17b106b Compare August 19, 2023 04:52

fee1-dead force-pushed the bindeps-registry2 branch from 17b106b to b90af3d Compare August 19, 2023 06:01

This was referenced Aug 24, 2023

bindeps: Enhance download_accessible to download all potential dependencies #12554

Closed

bindeps: Update crates.io to support binary dependencies #12555

Open

fee1-dead added 5 commits August 25, 2023 01:43

Add fields for bindeps on the registry

3d50f47

Query RustcTargetData during feature resolution

0863369

update tests

b2b34e4

format

6b65d06

apply suggestions

43dccc7

fee1-dead force-pushed the bindeps-registry2 branch from b90af3d to 43dccc7 Compare August 25, 2023 01:43

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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 25, 2023

bors merged commit e2c6a3e into rust-lang:master Aug 25, 2023
20 checks passed

fee1-dead deleted the bindeps-registry2 branch August 25, 2023 09:07

weihanglo mentioned this pull request Aug 26, 2023

Update cargo rust-lang/rust#115243

Merged

ehuss added this to the 1.74.0 milestone Sep 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support dependencies from registries for artifact dependencies, take 2 #12421

Support dependencies from registries for artifact dependencies, take 2 #12421

fee1-dead commented Jul 31, 2023

bors commented Aug 1, 2023

ehuss commented Aug 15, 2023

ehuss Aug 18, 2023

fee1-dead Aug 19, 2023

ehuss Aug 18, 2023

fee1-dead Aug 19, 2023

ehuss Aug 18, 2023

fee1-dead Aug 19, 2023

ehuss Aug 18, 2023

fee1-dead Aug 19, 2023

ehuss Aug 19, 2023

fee1-dead Aug 19, 2023

ehuss commented Aug 19, 2023

fee1-dead commented Aug 19, 2023

fee1-dead commented Aug 19, 2023

bors commented Aug 24, 2023

ehuss commented Aug 24, 2023

ehuss commented Aug 24, 2023

ehuss commented Aug 25, 2023

bors commented Aug 25, 2023

bors commented Aug 25, 2023

bors commented Aug 25, 2023

elchukc commented Oct 23, 2024

ehuss commented Oct 25, 2024

	/// into [`Summaries::raw_data`] which this isRegistryDependency an entry of.
	/// into [`Summaries::raw_data`] which this is an entry of.

		#[cargo_test]
		#[ignore = "broken, need artifact info in index"]

Support dependencies from registries for artifact dependencies, take 2 #12421

Support dependencies from registries for artifact dependencies, take 2 #12421

Conversation

fee1-dead commented Jul 31, 2023

bors commented Aug 1, 2023

ehuss commented Aug 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss commented Aug 19, 2023

fee1-dead commented Aug 19, 2023

fee1-dead commented Aug 19, 2023

bors commented Aug 24, 2023

ehuss commented Aug 24, 2023

ehuss commented Aug 24, 2023

ehuss commented Aug 25, 2023

bors commented Aug 25, 2023

bors commented Aug 25, 2023

bors commented Aug 25, 2023

elchukc commented Oct 23, 2024

ehuss commented Oct 25, 2024