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

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

Merged
merged 2 commits into from
Feb 26, 2023

Conversation

arlosi
Copy link
Contributor

@arlosi arlosi commented Feb 22, 2023

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
Copy link
Collaborator

rustbot commented Feb 22, 2023

r? @weihanglo

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

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

Worth a beta backport?

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

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

ehuss commented Feb 24, 2023

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
Copy link
Contributor Author

arlosi commented Feb 24, 2023

@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
Copy link
Contributor

bors commented Feb 26, 2023

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

@ehuss
Copy link
Contributor

ehuss commented Feb 26, 2023

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
Copy link
Contributor

bors commented Feb 26, 2023

📌 Commit ab726e7 has been approved by ehuss

It is now in the queue for this repository.

@bors 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
Copy link
Contributor

bors commented Feb 26, 2023

⌛ Testing commit ab726e7 with merge 964b730...

@bors
Copy link
Contributor

bors commented Feb 26, 2023

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

@bors bors merged commit 964b730 into rust-lang:master Feb 26, 2023
@weihanglo
Copy link
Member

Will do the backport part.

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

weihanglo pushed a commit to weihanglo/cargo that referenced this pull request Feb 26, 2023
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
weihanglo pushed a commit to weihanglo/cargo that referenced this pull request Feb 26, 2023
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
bors added a commit that referenced this pull request Feb 26, 2023
[beta-1.68] backport #11756

Beta backports:

- #11756

In order to make CI pass, the following PR are also cherry-picked:

- #11771
weihanglo added a commit to weihanglo/rust that referenced this pull request Feb 26, 2023
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)
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 27, 2023
…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`
weihanglo added a commit to weihanglo/rust that referenced this pull request Feb 28, 2023
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)
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 1, 2023
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`
@ehuss ehuss added this to the 1.69.0 milestone Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-nominated Nominated to backport to the beta branch. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
5 participants