refactor(SourceId): merge `name` and `alt_registry_key` into one enum #12675

weihanglo · 2023-09-15T09:12:32Z

What does this PR try to resolve?

Both name and alt_registry_key are mainly used for displaying.
We can safely merge them into one enum.
However, alt_registry_key is also used for telling if a SourceId is
from [registries] so we need to provide functions to distinguish that.

How should we test and review this PR?

A new test is added to prevent regressions like #12675 (comment).

rustbot · 2023-09-15T09:12:37Z

r? @epage

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

arlosi · 2023-09-15T15:41:13Z

src/cargo/sources/config.rs

@@ -239,7 +239,7 @@ restore the source replacement configuration to continue the build
        let mut srcs = Vec::new();
        if let Some(registry) = def.registry {
            let url = url(&registry, &format!("source.{}.registry", name))?;
-            srcs.push(SourceId::for_alt_registry(&url, &name)?);
+            srcs.push(SourceId::for_named_registry(&url, &name)?);


Previously alt_registry_key was only set for registries defined in the [registries] table, while this change uses registry_key for names in the [source] table as well.

I wish it wasn't possible to define remote registries using the [source] table, but it is. At minimum we need to change the doc comment on registry_key, since it indicates that it's only the registries table.

There are uses such as auth/mod.rs L432 that rely on the invariant that registry_key comes from the registries table. We'd need to be sure that case isn't possible for a SourceId that came from the sources table. Otherwise it would print misleading help messages, since cargo login doesn't work for sources defined in the sources table.

A way to not break the invariant is doing this 😆:

Suggested change

srcs.push(SourceId::for_named_registry(&url, &name)?);

srcs.push(SourceId::for_registry(&url)?);

I'll try to setup some test case to see whether the error message of [source] itself regresses or not.

I've ended up replacing it with for_registry.

It should have any regression regarding status console output. Cargo shows the original one.

The invariant of registry_key only from [registries] table is maintained. Calling for_registry() means registry from [source] never get a registry_key set.

What do people think? Is there any case I am missing?

ehuss · 2023-09-19T18:54:02Z

There is a small difference in the output in the following situation:

[source.crates-io]
replace-with = 'alternative'

[source.alternative]
registry = 'file:///path/to/registry'

With this, the old output would be:

    Updating `alternative` index
  Downloaded bar v0.0.1 (registry `alternative`)
  Downloaded 1 crate (180 B) in 0.00s
    Checking bar v0.0.1
    Checking foo v0.0.1 (/cargo/target/tmp/cit/t0/foo)
    Finished dev [unoptimized + debuginfo] target(s) in 0.06s

The output with this PR would be:

    Updating `/cargo/target/tmp/cit/t0/registry` index
  Downloaded bar v0.0.1 (registry `/cargo/target/tmp/cit/t0/registry`)
  Downloaded 1 crate (180 B) in 0.00s
    Checking bar v0.0.1
    Checking foo v0.0.1 (/cargo/target/tmp/cit/t0/foo)
    Finished dev [unoptimized + debuginfo] target(s) in 0.07s

This also applies to error messages, like:

error: crates-io is replaced with non-remote-registry source registry `alternative`;

versus:

error: crates-io is replaced with non-remote-registry source registry `/cargo/target/tmp/cit/t0/alternative-registry`;

I don't think that is a big loss, but it is a regression where it no longer tells you the name of the source.

@arlosi, are you OK with reviewing this?
@weihanglo, can you add a test that demonstrates something like the above? (I don't think replace-with is required, but I'm not sure).

arlosi

This looks pretty good now. I'm ok with the change in error message for remote registry sources. I'll do one more check to make sure there aren't any further impacts tomorrow.

arlosi · 2023-09-20T04:54:59Z

src/cargo/core/source_id.rs

    /// generates better messages for cargo.
    pub fn for_registry(url: &Url) -> CargoResult<SourceId> {
        let kind = Self::remote_source_kind(url);
        SourceId::new(kind, url.to_owned(), None)
    }

    /// Creates a `SourceId` from a remote registry URL with given name.
-    pub fn for_alt_registry(url: &Url, name: &str) -> CargoResult<SourceId> {
+    pub fn for_named_registry(url: &Url, name: &str) -> CargoResult<SourceId> {


Can we document that the name is an entry in the [registries] table?

Also consider renaming the argument to key to match the new function

arlosi · 2023-09-20T05:01:12Z

src/cargo/core/source_id.rs

-    alt_registry_key: Option<String>,
+    /// Name of the remote registry as defined in the `[registries]` table.
+    ///
+    /// WARNING: this is not always set for alt-registries when the name is not known.


Could you add something like
"The value for crates.io is crates-io"

arlosi · 2023-09-20T05:07:51Z

src/cargo/util/auth/mod.rs

    let name = if sid.is_crates_io() {
        Some(CRATES_IO_REGISTRY)
    } else {
-        sid.alt_registry_key()
+        sid.registry_key()
    };


It looks like registry_key should always be crates-io when is_crates_io() is true.

Can this if be removed and replaced with let name = sid.registry_key()?

There are a few other places with this pattern too.

weihanglo · 2023-09-21T05:16:35Z

Chose to use an enum to differentiate registries from [source] and [registries] tables. This should address the concern of error message regressions, as well as maintain the invariant.

src/cargo/core/source_id.rs

src/cargo/core/package_id.rs

Both `name` and `alt_registry_key` are mainly used for displaying. We can safely merge them into one enum. However, `alt_registry_key` is also used for telling if a SourceId is from `[registries]` so we need to provide functions to distinguish that.

`registry_key` should always be `crates-io` when `is_crates_io()` is true This also removes the test verifying `Debug` output, whose format doesn't need to be guaranteed.

arlosi · 2023-09-22T18:42:49Z

Thanks! Looks great.

@bors r+

bors · 2023-09-22T18:42:51Z

📌 Commit 23e91c3 has been approved by arlosi

It is now in the queue for this repository.

bors · 2023-09-22T18:42:58Z

⌛ Testing commit 23e91c3 with merge 25dcec9...

bors · 2023-09-22T19:41:12Z

☀️ Test successful - checks-actions
Approved by: arlosi
Pushing 25dcec9 to master...

Update cargo 11 commits in 414d9e3a6d8096f3e276234ce220c868767a8792..e6aabe8b3fcf639be3a5bf68e77853bd7b3fa27d 2023-09-22 07:03:57 +0000 to 2023-09-26 16:31:53 +0000 - Use full target spec for `cargo rustc --print --target` (rust-lang/cargo#12743) - feat(embedded): Hack in code fence support (rust-lang/cargo#12681) - chore(ci): Update Renovate schema (rust-lang/cargo#12741) - more specific registry index not found msg (rust-lang/cargo#12732) - docs: warn about upload timeout (rust-lang/cargo#12733) - Fix some typos (rust-lang/cargo#12730) - upgrade gitoxide to v0.54 (rust-lang/cargo#12731) - Update target-arch-aware crates to support mips r6 targets (rust-lang/cargo#12720) - Buffer console status messages. (rust-lang/cargo#12727) - Fix spurious errors with networking tests. (rust-lang/cargo#12726) - refactor(SourceId): merge `name` and `alt_registry_key` into one enum (rust-lang/cargo#12675) r? ghost

rustbot assigned epage Sep 15, 2023

weihanglo force-pushed the source-id branch from 858c012 to 2ded388 Compare September 15, 2023 09:58

arlosi reviewed Sep 15, 2023

View reviewed changes

arlosi reviewed Sep 20, 2023

View reviewed changes

arlosi assigned arlosi and unassigned epage Sep 20, 2023

test: verify source replacement displays registry key

a30d9fd

weihanglo force-pushed the source-id branch 3 times, most recently from 12acc02 to 630bbdb Compare September 21, 2023 05:11

weihanglo changed the title ~~refactor: remove SourceId.name field~~ refactor(SourceId): merge name and alt_registry_key into one enum Sep 21, 2023

arlosi reviewed Sep 21, 2023

View reviewed changes

src/cargo/core/source_id.rs Outdated Show resolved Hide resolved

src/cargo/core/package_id.rs Show resolved Hide resolved

weihanglo added 2 commits September 22, 2023 14:36

refactor: use registry_key for getting crates.io key

23e91c3

`registry_key` should always be `crates-io` when `is_crates_io()` is true This also removes the test verifying `Debug` output, whose format doesn't need to be guaranteed.

weihanglo force-pushed the source-id branch from 630bbdb to 23e91c3 Compare September 22, 2023 06:52

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 Sep 22, 2023

bors merged commit 25dcec9 into rust-lang:master Sep 22, 2023
19 checks passed

weihanglo mentioned this pull request Sep 27, 2023

Update cargo rust-lang/rust#116189

Merged

ehuss added this to the 1.74.0 milestone Sep 30, 2023

weihanglo deleted the source-id branch October 20, 2023 21:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(SourceId): merge `name` and `alt_registry_key` into one enum #12675

refactor(SourceId): merge `name` and `alt_registry_key` into one enum #12675

weihanglo commented Sep 15, 2023 •

edited

Loading

rustbot commented Sep 15, 2023

arlosi Sep 15, 2023

weihanglo Sep 16, 2023

weihanglo Sep 19, 2023

ehuss commented Sep 19, 2023

arlosi left a comment

arlosi Sep 20, 2023

arlosi Sep 20, 2023

arlosi Sep 20, 2023

weihanglo commented Sep 21, 2023

arlosi commented Sep 22, 2023

bors commented Sep 22, 2023

bors commented Sep 22, 2023

bors commented Sep 22, 2023

	srcs.push(SourceId::for_named_registry(&url, &name)?);
	srcs.push(SourceId::for_registry(&url)?);

refactor(SourceId): merge name and alt_registry_key into one enum #12675

refactor(SourceId): merge name and alt_registry_key into one enum #12675

Conversation

weihanglo commented Sep 15, 2023 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

rustbot commented Sep 15, 2023

arlosi Sep 15, 2023

Choose a reason for hiding this comment

weihanglo Sep 16, 2023

Choose a reason for hiding this comment

weihanglo Sep 19, 2023

Choose a reason for hiding this comment

ehuss commented Sep 19, 2023

arlosi left a comment

Choose a reason for hiding this comment

arlosi Sep 20, 2023

Choose a reason for hiding this comment

arlosi Sep 20, 2023

Choose a reason for hiding this comment

arlosi Sep 20, 2023

Choose a reason for hiding this comment

weihanglo commented Sep 21, 2023

arlosi commented Sep 22, 2023

bors commented Sep 22, 2023

bors commented Sep 22, 2023

bors commented Sep 22, 2023

refactor(SourceId): merge `name` and `alt_registry_key` into one enum #12675

refactor(SourceId): merge `name` and `alt_registry_key` into one enum #12675

weihanglo commented Sep 15, 2023 •

edited

Loading