Fix Cargo removing the sparse+ prefix from sparse URLs in .crates.toml by arlosi · Pull Request #11756 · rust-lang/cargo

arlosi · 2023-02-22T16:21:53Z

The URL associated with a SourceId for a sparse registry includes the sparse+ prefix in the URL to differentiate from git (non-sparse) registries.

SourceId::from_url was not including the sparse+ prefix of sparse registry URLs on construction, which caused roundtrips of as_url and from_url to fail by losing the prefix.

Fixes #11751 by:

Including the prefix in the URL
Adding a test that verifies round-trip behavior for sparse SourceIds
Modifying CanonicalUrl so it no longer considers sparse+ and non-sparse+ URLs to be equivalent

rustbot · 2023-02-22T16:21:58Z

r? @weihanglo

(rustbot has picked a reviewer for you, use r? to override)

weihanglo · 2023-02-22T17:00:27Z

Worth a beta backport?

weihanglo

It's not immediately clear what the expected and valid input/output URL is for each function.

From my understanding, SourceId::from_url including protos (git, registry, sparse, path). So do for_alt_registry and for_registry, but not for_git.

For SourceId::new, only SparseRegistry should have proto+ prefix.

Should it desire a doc or thorough unit tests for each kind and function?

weihanglo

Anyhow this looks correct to me. It is also safe from #11487 as we already have regression tests.

@rust-lang/cargo could anyone help me double check this?

ehuss · 2023-02-24T04:03:11Z

I'm not sure I can anticipate the consequences of this change. This seems like it is really subtle stuff. I don't understand why sparse is keeping the sparse+ prefix in the SourceId.url when no other source kinds do that. I looked at what it might take to change that, and it's not pretty. Can you explain why it is done this way? Is it feasible to not include the sparse+ prefix so that it behaves the same as all the other kinds? If not, can that be clarified as to why it is done differently?

Why was CanonicalUrl ignoring the sparse+ prefix in the first place?

To clarify my understanding, is the is_crates_io() function the key for cargo treating the crates.io git and sparse index the same? And that the PackageId skips rendering the registry when it is crates.io?

I'd like to see more comments explaining some of these subtleties.

Can you add a test that covers the example given in the issue? Perhaps something like this:

#[cargo_test]
fn sparse_install() {
    // Checks for an issue where uninstalling from something installed
    // from a sparse registry corrupted the source IDs of other entries.
    // See https://github.com/rust-lang/cargo/issues/11751
    let _registry = registry::RegistryBuilder::new().http_index().build();

    pkg("foo", "0.0.1");
    pkg("bar", "0.0.1");

    cargo_process("install foo --registry dummy-registry")
        .with_stderr(
            "\
[UPDATING] `dummy-registry` index
[DOWNLOADING] crates ...
[DOWNLOADED] foo v0.0.1 (registry `dummy-registry`)
[INSTALLING] foo v0.0.1 (registry `dummy-registry`)
[UPDATING] `dummy-registry` index
[COMPILING] foo v0.0.1 (registry `dummy-registry`)
[FINISHED] release [optimized] target(s) in [..]
[INSTALLING] [ROOT]/home/.cargo/bin/foo
[INSTALLED] package `foo v0.0.1 (registry `dummy-registry`)` (executable `foo`)
[WARNING] be sure to add `[..]` to your PATH to be able to run the installed binaries
",
        )
        .run();
    assert_has_installed_exe(cargo_home(), "foo");
    let assert_v1 = |expected| {
        let v1 = fs::read_to_string(paths::home().join(".cargo/.crates.toml")).unwrap();
        assert_match_exact(expected, &v1);
    };
    assert_v1(
        r#"[v1]
"foo 0.0.1 (sparse+http://127.0.0.1:[..]/index/)" = ["foo"]
"#,
    );
    cargo_process("install bar").run();
    assert_has_installed_exe(cargo_home(), "bar");
    assert_v1(
        r#"[v1]
"bar 0.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = ["bar"]
"foo 0.0.1 (sparse+http://127.0.0.1:[..]/index/)" = ["foo"]
"#,
    );

    cargo_process("uninstall bar")
        .with_stderr("[REMOVING] [CWD]/home/.cargo/bin/bar[EXE]")
        .run();
    assert_has_not_installed_exe(cargo_home(), "bar");
    assert_v1(
        r#"[v1]
"foo 0.0.1 (sparse+http://127.0.0.1:[..]/index/)" = ["foo"]
"#,
    );
    cargo_process("uninstall foo")
        .with_stderr("[REMOVING] [CWD]/home/.cargo/bin/foo[EXE]")
        .run();
    assert_has_not_installed_exe(cargo_home(), "foo");
    assert_v1(
        r#"[v1]
"#,
    );
}

arlosi · 2023-02-24T21:27:07Z

@ehuss thanks for the extra test, I've added it. I also added another comment that attempts to explain why sparse+ URLs are different.

            // Sparse URLs are different because they store the kind prefix (sparse+)
            // in the URL. This is because the prefix is necessary to differentiate
            // from regular registries (git-based). The sparse+ prefix is included
            // everywhere, including user-facing locations such as the `config.toml`
            // file that defines the registry and whenever Cargo displays it to the user.

Why was CanonicalUrl ignoring the sparse+ prefix in the first place?

We were planning on doing an autodetection mechanism so the sparse+ prefix would be optional.

To clarify my understanding, is the is_crates_io() function the key for cargo treating the crates.io git and sparse index the same? And that the PackageId skips rendering the registry when it is crates.io?

The is_crates_io() function is the key for making it display the same. The key to making the sparse index of crates.io equivalent to the git index is source replacement, and the crates_io_maybe_sparse_http() function, which returns the sparse crates.io source id (if it's enabled).

bors · 2023-02-26T00:42:25Z

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

ehuss · 2023-02-26T02:54:57Z

I pushed a fix for Windows.

I still feel like the subtleties here are potentially fragile, but I don't have any particular ideas that don't involve massive changes.

@bors r+

@weihanglo Yea, I think this would be good to backport. Can someone take care of that?

(BTW, thanks @arlosi for the quick fix!)

bors · 2023-02-26T02:54:59Z

📌 Commit ab726e7 has been approved by ehuss

It is now in the queue for this repository.

bors · 2023-02-26T02:55:05Z

⌛ Testing commit ab726e7 with merge 964b730...

bors · 2023-02-26T03:34:26Z

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

weihanglo · 2023-02-26T08:26:45Z

Will do the backport part.

For consolidating the logic, maybe we should test against an exhaustive list of possible URLs here.

Fix Cargo removing the sparse+ prefix from sparse URLs in .crates.toml The URL associated with a `SourceId` for a sparse registry includes the `sparse+` prefix in the URL to differentiate from git (non-sparse) registries. `SourceId::from_url` was not including the `sparse+` prefix of sparse registry URLs on construction, which caused roundtrips of `as_url` and `from_url` to fail by losing the prefix. Fixes rust-lang#11751 by: * Including the prefix in the URL * Adding a test that verifies round-trip behavior for sparse `SourceId`s * Modifying `CanonicalUrl` so it no longer considers `sparse+` and non-`sparse+` URLs to be equivalent

[beta-1.68] backport #11756 Beta backports: - #11756 In order to make CI pass, the following PR are also cherry-picked: - #11771

3 commits in ddf05ad7a66f4cfbe79d7692b84aa144c1aac34d..115f34552518a2f9b96d740192addbac1271e7e6 2023-02-09 03:13:43 +0000 to 2023-02-26 15:07:29 +0000 - [beta-1.68] backport rust-lang/cargo#11756 (rust-lang/cargo#11773) - [beta-1.68] backport rust-lang/cargo#11759 (rust-lang/cargo#11760) - [beta-1.68] backport rust-lang/cargo#11733 (rust-lang/cargo#11735)

…nglo [beta-1.68] cargo beta backports 3 commits in ddf05ad7a66f4cfbe79d7692b84aa144c1aac34d..115f34552518a2f9b96d740192addbac1271e7e6 2023-02-09 03:13:43 +0000 to 2023-02-26 15:07:29 +0000 - [beta-1.68] backport rust-lang/cargo#11756 (rust-lang/cargo#11773) - [beta-1.68] backport rust-lang/cargo#11759 (rust-lang/cargo#11760) - [beta-1.68] backport rust-lang/cargo#11733 (rust-lang/cargo#11735) r? `@ghost`

10 commits in 9d5b32f503fc099c4064298465add14d4bce11e6..9880b408a3af50c08fab3dbf4aa2a972df71e951 2023-02-22 23:04:16 +0000 to 2023-02-28 19:39:39 +0000 - bump jobserver to respect `--jobserver-auth=fifo:PATH` (rust-lang/cargo#11767) - Addition of support for -F as an alias for --features (rust-lang/cargo#11774) - Added documentation for the configuration discovery of `cargo install` to the man pages (rust-lang/cargo#11763) - Fix Cargo removing the sparse+ prefix from sparse URLs in .crates.toml (rust-lang/cargo#11756) - Fix warning with tempfile (rust-lang/cargo#11771) - Error message for transitive artifact dependencies with targets the package doesn't directly interact with (rust-lang/cargo#11643) - Fix tests with nondeterministic ordering (rust-lang/cargo#11766) - Make some blocking tests non-blocking (rust-lang/cargo#11650) - Suggest cargo add when installing library crate (rust-lang/cargo#11410) - chore: bump is-terminal to 0.4.4 (rust-lang/cargo#11759)

Update cargo 10 commits in 9d5b32f503fc099c4064298465add14d4bce11e6..9880b408a3af50c08fab3dbf4aa2a972df71e951 2023-02-22 23:04:16 +0000 to 2023-02-28 19:39:39 +0000 - bump jobserver to respect `--jobserver-auth=fifo:PATH` (rust-lang/cargo#11767) - Addition of support for -F as an alias for --features (rust-lang/cargo#11774) - Added documentation for the configuration discovery of `cargo install` to the man pages (rust-lang/cargo#11763) - Fix Cargo removing the sparse+ prefix from sparse URLs in .crates.toml (rust-lang/cargo#11756) - Fix warning with tempfile (rust-lang/cargo#11771) - Error message for transitive artifact dependencies with targets the package doesn't directly interact with (rust-lang/cargo#11643) - Fix tests with nondeterministic ordering (rust-lang/cargo#11766) - Make some blocking tests non-blocking (rust-lang/cargo#11650) - Suggest cargo add when installing library crate (rust-lang/cargo#11410) - chore: bump is-terminal to 0.4.4 (rust-lang/cargo#11759) r? `@ghost`

rustbot assigned weihanglo Feb 22, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 22, 2023

arlosi added the beta-nominated Nominated to backport to the beta branch. label Feb 22, 2023

weihanglo reviewed Feb 22, 2023

View reviewed changes

weihanglo approved these changes Feb 22, 2023

View reviewed changes

arlosi force-pushed the sparse-uninstall branch from b9a3cf4 to 9b0317b Compare February 24, 2023 21:15

arlosi and others added 2 commits February 25, 2023 18:07

Fix Cargo removing the sparse+ prefix from sparse URLs in .crates.toml

16ad1f2

Fix test for Windows

ab726e7

ehuss force-pushed the sparse-uninstall branch from ed34695 to ab726e7 Compare February 26, 2023 02:08

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 Feb 26, 2023

bors merged commit 964b730 into rust-lang:master Feb 26, 2023

weihanglo mentioned this pull request Feb 26, 2023

[beta-1.68] backport #11756 #11773

Merged

bors added a commit that referenced this pull request Feb 26, 2023

Auto merge of #11773 - weihanglo:beta-backport, r=ehuss

115f345

[beta-1.68] backport #11756 Beta backports: - #11756 In order to make CI pass, the following PR are also cherry-picked: - #11771

weihanglo mentioned this pull request Feb 26, 2023

[beta-1.68] cargo beta backports rust-lang/rust#108508

Merged

weihanglo mentioned this pull request Feb 28, 2023

Update cargo rust-lang/rust#108582

Merged

ehuss added this to the 1.69.0 milestone Mar 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Cargo removing the sparse+ prefix from sparse URLs in .crates.toml#11756

Fix Cargo removing the sparse+ prefix from sparse URLs in .crates.toml#11756
bors merged 2 commits intorust-lang:masterfrom
arlosi:sparse-uninstall

arlosi commented Feb 22, 2023

Uh oh!

rustbot commented Feb 22, 2023

Uh oh!

weihanglo commented Feb 22, 2023

Uh oh!

weihanglo left a comment

Uh oh!

weihanglo left a comment

Uh oh!

ehuss commented Feb 24, 2023

Uh oh!

arlosi commented Feb 24, 2023

Uh oh!

bors commented Feb 26, 2023

Uh oh!

ehuss commented Feb 26, 2023 •

edited

Loading

Uh oh!

bors commented Feb 26, 2023

Uh oh!

bors commented Feb 26, 2023

Uh oh!

bors commented Feb 26, 2023

Uh oh!

weihanglo commented Feb 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

arlosi commented Feb 22, 2023

Uh oh!

rustbot commented Feb 22, 2023

Uh oh!

weihanglo commented Feb 22, 2023

Uh oh!

weihanglo left a comment

Choose a reason for hiding this comment

Uh oh!

weihanglo left a comment

Choose a reason for hiding this comment

Uh oh!

ehuss commented Feb 24, 2023

Uh oh!

arlosi commented Feb 24, 2023

Uh oh!

bors commented Feb 26, 2023

Uh oh!

ehuss commented Feb 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Feb 26, 2023

Uh oh!

bors commented Feb 26, 2023

Uh oh!

bors commented Feb 26, 2023

Uh oh!

weihanglo commented Feb 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ehuss commented Feb 26, 2023 •

edited

Loading