Fail fast when package version is already published

[rushmorem]

For example instead of validating the version when uploading, let's do so before we even start building the project, or better yet launch the build process concurrently as we check the version. I just ran cargo publish, waited for a while while the build was in progress only to get an error message saying the version was already published on crates.io.

See #4377 for more fail-fast cases

[epage]

The "version check" is us just POSTing the .crate file and it failing from what I can tell. I think it'd be reasonable for us to check the registry before the packaging step. This would then also work in dry-run mode.

For anyone wondering, the code for this is at

cargo/src/cargo/ops/registry/publish.rs

Line 57 in 71de58d

pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> {

[linyihai]

After some search, the crate-io api is here.
There is no suitable api for checking crate info like email, version an so on before the crate final pushlish.
As the issue mention：Pre-flight validation before uploading and before building the crate is a more reasonable way, but it stagnation since 2020.

[epage]

For this issue, I was scoping my comment to just the version. We have #4377 for generally tracking these more general validation rules.

EDIT: We also have #8221

[stupendoussuperpowers]

I can add a simple registry.search() here and check for package name and version in the result. should I just proceed with that, or should there be a separate function for validating all the required items listed in #4377.

[stupendoussuperpowers]

I'll start a PR if this implementation makes sense:

needed to add a helper to registry to check if version exists.

pub fn does_version_exist(&mut self, query: &str, version: &str) -> Result<bool> {
    let formatted_query = format!("/crates/{}/{}", query, version);
  
    let body = self.req(&format!("{}", formatted_query), None, Auth::Unauthorized);
  
    match body {
       Ok(_v) => Ok(true),
       Err(_e) => Ok(false),
    }
}

in the publish fn, we make the following check:

    let version_exists = registry.does_version_exist(&pkg.name(), &ver)?;

    if version_exists {
        bail!(
            "crate {} already has version {}. Aborting publish.",
            pkg.name(),
            pkg.version()
        );
    }

[epage]

Id instead look at the wait-for-publish code to see how it does a lookup

[stupendoussuperpowers]

wait for publish creates a Source using reg_id and config, and then queries it with package name and version. we could do this in publish, but I am not sure if it's the best pattern - seems a bit repetitive to create a Source from the same parameters twice in a single flow.

[stupendoussuperpowers]

Also, currently we have don't have tests for publish apart from the default one. Should a PR for this only consist of this modification?

[epage]

We don't necessarily have to take all of the code line for line, including creating a fresh Source, but the idea of checking the Index, rather than the registry API, and just using a regular query for it.

Also, currently we have don't have tests for publish apart from the default one. Should a PR for this only consist of this modification?

I'm not too sure what you mean by this. We should have a test added that shows the behavior. Ideally, a PR would have two commits, the first being a test that shows the current behavior and the second that changes the behavior and updates the test to reflect that.

[stupendoussuperpowers]

Got it. Will make the changes.

Disregard the test part, I was looking at the wrong place for existing publish tests. Will add a new test for this changed behaviour.

[stupendoussuperpowers]

@epage,

I have figured out the code change to make to fail fast with duplicate versions.

I am running into an issue with writing a test for this. By the looks of it TestRegistry doesn't store the test packages once we run publish, hence we run into no issues when trying to publish a package with a duplicate version, it just goes ahead and "publishes" it anyway.

Is there some way around this?

[epage]

There are two ways of testing publish in the testsuite. One just writes the API request to disk but does nothing. The other actually somewhat processes it and makes it work. The latter works sufficiently for the "wait for publish" code to work so I assume it would work in this case as well.

[epage]

With us having #4377, I'm going to narrow the scope to the example given in the description.