r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Some tests with alt_registry are failing.

When we query the index for duplicate version, it prints out an extra "Updating [..] index" line. Which is missing in those test cases.

Should I be modifying the failing tests to add that line, or is there a different way to run the query where this log line is not outputted?

If the fix does change the behaviour of those test cases, then assertions of printing status seems reasonable to me.

Note that I haven't looked into the fix itself.

When we query the index for duplicate version, it prints out an extra "Updating [..] index" line. Which is missing in those test cases.

Should I be modifying the failing tests to add that line, or is there a different way to run the query where this log line is not outputted?

Not looked into the details of when this message will show up or not but we likely don't need a new instance of SourceConfigMap and if we can reuse the way to look up registeries with other code, that'd likely help.

wait-for-publish has to go through some extra hoops because it needs to avoid reusing cached results.

The easy fix I found was to use source.set_quiet(true) - tests are passing with this.

I am also not able to find any other source present in this flow that we can re-use.

start_fetch() is supposed to be called only once per HttpRemote instance, I am unsure why creating a new source here is causing it to create a different instance for alternate registries but not for crates-io.

The first update in both cases happens when we create the registry object here - publish.rs#L130

For alternate registry upon querying it causes start_fetch() to be triggered again.

Any insight into how alternate registry queries work would be helpful. For now we can also continue with just adding set_quiet(true)

Hey, did anyone get a chance to check this?

In summary -

1. We can use source.set_quiet(true) which makes the alternate publish pass
2. I'll need some guidance into why there's different outputs for crates-io and an alternate registry

I'll need some guidance into why there's different outputs for crates-io and an alternate registry

Could you show us the difference? Let us know when this PR is ready for review :)

Somehow I remembered this issue #13397. While this PR just make it fail fast, it might be something to consider in the future.

@stupendoussuperpowers Will you provide the difference in output asked for above?

start_fetch() is supposed to be called only once per HttpRemote instance, I am unsure why creating a new source here is causing it to create a different instance for alternate registries but not for crates-io.

The first update in both cases happens when we create the registry object here - publish.rs#L130

For alternate registry upon querying it causes start_fetch() to be triggered again.

Any insight into how alternate registry queries work would be helpful. For now we can also continue with just adding set_quiet(true)

The above comment is the depth to which I was able to analyze the issue.

The symptom for this is -

When I added the code to check for existing version of the package, it logged an extra "Updating [..] index" line IF used with an alternate registry. It did not do this with crates-io. This causes certain alternate registry tests to fail since the output doesn't expect "Updating [..] index" to be present twice.

The multiple Updating messages do make sense in a way. The first one is notifying about fetching /index/config.json, and the second one about /index/3/f/foo. If these fetches were running in the same RegistrySource there would only be one message, but they aren't. One is made within super::registry() and one is made just after in this PR in let mut source = SourceConfigMap::empty(opts.gctx)?.load(reg_ids.original, &HashSet::new())?;.

I tried reusing the first one, but that broke the publish::duplicate_version test.

But that was fixed by changing

    let query = Dependency::parse(pkg.name(), Some(&ver), reg_ids.original)?;

to

    let query = Dependency::parse(pkg.name(), Some(&ver), reg_ids.replacement)?;

Here's a full fix:

diff.txt

@stupendoussuperpowers Will you try to apply this?

thanks @torhovland for the help, let me try and apply this patch. will submit a patched PR.

Hi! I already applied the fix and submitted it in #14338, but it turns out there's yet another test that demands some work. You can certainly look into that if you want.

Superseded by #14338.