Bail publish job before packaging and upload

[stupendoussuperpowers]

Fixes #3662

This PR adds a check for existing version in the registry before we package and upload the crate.

With this PR we get an error pointing out that the same version exists in registry, and we abort the publish job.

Also adds tests for the same.

[rustbot]

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

[stupendoussuperpowers]

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?

[weihanglo]

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.

[epage]

nit:
"crate {name}@{version} is already present in {registry}"
(have not looked into what "registery" variable would be appropriate to lit here)

• Specifying the registry might be helpful in case someone is publishing to multiple as that gives them a chance to think "Oh whoops, I meant this other one"
• We likely don't need an "Aborting publish" as thats what an error is
• The current phrasing is a little awkward (saying a crate has a version reads a little funny to me)

[stupendoussuperpowers]

registry variable here can be source.describe() which would be "crates.io index" in this case.

[epage]

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.

[stupendoussuperpowers]

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)

[stupendoussuperpowers]

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

[weihanglo]

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.

[torhovland]

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

[stupendoussuperpowers]

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.

[torhovland]

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.

[torhovland]

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?

[stupendoussuperpowers]

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

[torhovland]

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.

[torhovland]

Superseded by #14338.