Repository: rust-lang/cargo (rust)

Issue:
Fail fast when package version is already published
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

Pull request:
bail before packaging on same version 
<!-- homu-ignore:start -->
<!--
Thanks for submitting a pull request 🎉! Here are some tips for you:

* If this is your first contribution, read "Cargo Contribution Guide" first:
  https://doc.crates.io/contrib/
* Run `cargo fmt --all` to format your code changes.
* Small commits and pull requests are always preferable and easy to review.
* If your idea is large and needs feedback from the community, read how:
  https://doc.crates.io/contrib/process/#working-on-large-features
* Cargo takes care of compatibility. Read our design principles:
  https://doc.crates.io/contrib/design.html
* When changing help text of cargo commands, follow the steps to generate docs:
  https://github.com/rust-lang/cargo/tree/master/src/doc#building-the-man-pages
* If your PR is not finished, set it as "draft" PR or add "WIP" in its title.
* It's ok to use the CI resources to test your PR, but please don't abuse them.

### What does this PR try to resolve?

Explain the motivation behind this change.
A clear overview along with an in-depth explanation are helpful.

You can use `Fixes #<issue number>` to associate this PR to an existing issue.

### How should we test and review this PR?

Demonstrate how you test this change and guide reviewers through your PR.
With a smooth review process, a pull request usually gets reviewed quicker.

If you don't know how to write and run your tests, please read the guide:
https://doc.crates.io/contrib/tests

### Additional information

Other information you want to mention in this PR, such as prior arts,
future extensions, an unresolved problem, or a TODO list.
-->
<!-- homu-ignore:end -->


Fixes #3662. Cleaned up commits from #14338.

Patch:
diff --git a/src/cargo/ops/registry/login.rs b/src/cargo/ops/registry/login.rs
index 4087317bb..34e78e1ed 100644
--- a/src/cargo/ops/registry/login.rs
+++ b/src/cargo/ops/registry/login.rs
@@ -34,7 +34,7 @@ pub fn registry_login(
         false,
         None,
     ) {
-        Ok(registry) => Some(format!("{}/me", registry.host())),
+        Ok((registry, _)) => Some(format!("{}/me", registry.host())),
         Err(e) if e.is::<AuthorizationError>() => e
             .downcast::<AuthorizationError>()
             .unwrap()
diff --git a/src/cargo/ops/registry/mod.rs b/src/cargo/ops/registry/mod.rs
index 93b48b6f6..ea49a5dfa 100644
--- a/src/cargo/ops/registry/mod.rs
+++ b/src/cargo/ops/registry/mod.rs
@@ -120,14 +120,14 @@ impl RegistryCredentialConfig {
 ///   `registry`, or `index` are set, then uses `crates-io`.
 /// * `force_update`: If `true`, forces the index to be updated.
 /// * `token_required`: If `true`, the token will be set.
-fn registry(
-    gctx: &GlobalContext,
+fn registry<'gctx>(
+    gctx: &'gctx GlobalContext,
     source_ids: &RegistrySourceIds,
     token_from_cmdline: Option<Secret<&str>>,
     reg_or_index: Option<&RegistryOrIndex>,
     force_update: bool,
     token_required: Option<Operation<'_>>,
-) -> CargoResult<Registry> {
+) -> CargoResult<(Registry, RegistrySource<'gctx>)> {
     let is_index = reg_or_index.map(|v| v.is_index()).unwrap_or_default();
     if is_index && token_required.is_some() && token_from_cmdline.is_none() {
         bail!("command-line argument --index requires --token to be specified");
@@ -136,9 +136,9 @@ fn registry(
         auth::cache_token_from_commandline(gctx, &source_ids.original, token);
     }
 
+    let mut src = RegistrySource::remote(source_ids.replacement, &HashSet::new(), gctx)?;
     let cfg = {
         let _lock = gctx.acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?;
-        let mut src = RegistrySource::remote(source_ids.replacement, &HashSet::new(), gctx)?;
         // Only update the index if `force_update` is set.
         if force_update {
             src.invalidate_cache()
@@ -170,11 +170,9 @@ fn registry(
         None
     };
     let handle = http_handle(gctx)?;
-    Ok(Registry::new_handle(
-        api_host,
-        token,
-        handle,
-        cfg.auth_required,
+    Ok((
+        Registry::new_handle(api_host, token, handle, cfg.auth_required),
+        src,
     ))
 }
 
diff --git a/src/cargo/ops/registry/owner.rs b/src/cargo/ops/registry/owner.rs
index a5ff86594..7c8246fdf 100644
--- a/src/cargo/ops/registry/owner.rs
+++ b/src/cargo/ops/registry/owner.rs
@@ -36,7 +36,7 @@ pub fn modify_owners(gctx: &GlobalContext, opts: &OwnersOptions) -> CargoResult<
 
     let operation = Operation::Owners { name: &name };
     let source_ids = super::get_source_id(gctx, opts.reg_or_index.as_ref())?;
-    let mut registry = super::registry(
+    let (mut registry, _) = super::registry(
         gctx,
         &source_ids,
         opts.token.as_ref().map(Secret::as_deref),
diff --git a/src/cargo/ops/registry/publish.rs b/src/cargo/ops/registry/publish.rs
index 527b0a3fb..8ff92920c 100644
--- a/src/cargo/ops/registry/publish.rs
+++ b/src/cargo/ops/registry/publish.rs
@@ -31,11 +31,13 @@ use crate::core::PackageIdSpecQuery;
 use crate::core::SourceId;
 use crate::core::Workspace;
 use crate::ops;
+use crate::ops::registry::RegistrySourceIds;
 use crate::ops::PackageOpts;
 use crate::ops::Packages;
 use crate::ops::RegistryOrIndex;
 use crate::sources::source::QueryKind;
 use crate::sources::source::Source;
+use crate::sources::RegistrySource;
 use crate::sources::SourceConfigMap;
 use crate::sources::CRATES_IO_REGISTRY;
 use crate::util::auth;
@@ -45,6 +47,7 @@ use crate::util::toml::prepare_for_publish;
 use crate::util::Graph;
 use crate::util::Progress;
 use crate::util::ProgressStyle;
+use crate::util::VersionExt as _;
 use crate::CargoResult;
 use crate::GlobalContext;
 
@@ -115,7 +118,7 @@ pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> {
     // This is only used to confirm that we can create a token before we build the package.
     // This causes the credential provider to be called an extra time, but keeps the same order of errors.
     let source_ids = super::get_source_id(opts.gctx, reg_or_index.as_ref())?;
-    let mut registry = super::registry(
+    let (mut registry, mut source) = super::registry(
         opts.gctx,
         &source_ids,
         opts.token.as_ref().map(Secret::as_deref),
@@ -124,9 +127,15 @@ pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> {
         Some(Operation::Read).filter(|_| !opts.dry_run),
     )?;
 
-    // Validate all the packages before publishing any of them.
-    for (pkg, _) in &pkgs {
-        verify_dependencies(pkg, &registry, source_ids.original)?;
+    {
+        let _lock = opts
+            .gctx
+            .acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?;
+
+        for (pkg, _) in &pkgs {
+            verify_unpublished(pkg, &mut source, &source_ids)?;
+            verify_dependencies(pkg, &registry, source_ids.original)?;
+        }
     }
 
     let pkg_dep_graph = ops::cargo_package::package_with_dep_graph(
@@ -355,6 +364,36 @@ fn poll_one_package(
     Ok(!summaries.is_empty())
 }
 
+fn verify_unpublished(
+    pkg: &Package,
+    source: &mut RegistrySource<'_>,
+    source_ids: &RegistrySourceIds,
+) -> CargoResult<()> {
+    let query = Dependency::parse(
+        pkg.name(),
+        Some(&pkg.version().to_exact_req().to_string()),
+        source_ids.replacement,
+    )?;
+    let duplicate_query = loop {
+        match source.query_vec(&query, QueryKind::Exact) {
+            std::task::Poll::Ready(res) => {
+                break res?;
+            }
+            std::task::Poll::Pending => source.block_until_ready()?,
+        }
+    };
+    if !duplicate_query.is_empty() {
+        bail!(
+            "crate {}@{} already exists on {}",
+            pkg.name(),
+            pkg.version(),
+            source.describe()
+        );
+    }
+
+    Ok(())
+}
+
 fn verify_dependencies(
     pkg: &Package,
     registry: &Registry,
diff --git a/src/cargo/ops/registry/search.rs b/src/cargo/ops/registry/search.rs
index 41e12e36e..652730f1f 100644
--- a/src/cargo/ops/registry/search.rs
+++ b/src/cargo/ops/registry/search.rs
@@ -21,7 +21,7 @@ pub fn search(
     limit: u32,
 ) -> CargoResult<()> {
     let source_ids = super::get_source_id(gctx, reg_or_index.as_ref())?;
-    let mut registry =
+    let (mut registry, _) =
         super::registry(gctx, &source_ids, None, reg_or_index.as_ref(), false, None)?;
     let (crates, total_crates) = registry.search(query, limit).with_context(|| {
         format!(
diff --git a/src/cargo/ops/registry/yank.rs b/src/cargo/ops/registry/yank.rs
index 523c21bf8..f46b9332f 100644
--- a/src/cargo/ops/registry/yank.rs
+++ b/src/cargo/ops/registry/yank.rs
@@ -47,7 +47,7 @@ pub fn yank(
         }
     };
     let source_ids = super::get_source_id(gctx, reg_or_index.as_ref())?;
-    let mut registry = super::registry(
+    let (mut registry, _) = super::registry(
         gctx,
         &source_ids,
         token.as_ref().map(Secret::as_deref),


Test patch:
diff --git a/tests/testsuite/credential_process.rs b/tests/testsuite/credential_process.rs
index bba657fe8..391cdb33b 100644
--- a/tests/testsuite/credential_process.rs
+++ b/tests/testsuite/credential_process.rs
@@ -544,6 +544,31 @@ You may press ctrl-c [..]
         .with_stderr_data(output)
         .run();
 
+    let output_non_independent = r#"[UPDATING] `alternative` index
+{"v":1,"registry":{"index-url":"[..]","name":"alternative"},"kind":"get","operation":"read"}
+[PACKAGING] foo v0.1.1 ([ROOT]/foo)
+[PACKAGED] 3 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
+[UPLOADING] foo v0.1.1 ([ROOT]/foo)
+{"v":1,"registry":{"index-url":"[..]","name":"alternative"},"kind":"get","operation":"publish","name":"foo","vers":"0.1.1","cksum":"[..]"}
+[UPLOADED] foo v0.1.1 to registry `alternative`
+[NOTE] waiting [..]
+You may press ctrl-c [..]
+[PUBLISHED] foo v0.1.1 at registry `alternative`
+"#;
+
+    p.change_file(
+        "Cargo.toml",
+        r#"
+        [package]
+        name = "foo"
+        version = "0.1.1"
+        edition = "2015"
+        description = "foo"
+        license = "MIT"
+        homepage = "https://example.com/"
+    "#,
+    );
+
     p.change_file(
         ".cargo/config.toml",
         &format!(
@@ -557,7 +582,7 @@ You may press ctrl-c [..]
     );
 
     p.cargo("publish --registry alternative --no-verify")
-        .with_stderr_data(output)
+        .with_stderr_data(output_non_independent)
         .run();
 }
 
diff --git a/tests/testsuite/publish.rs b/tests/testsuite/publish.rs
index af071ae08..01a4cd0a9 100644
--- a/tests/testsuite/publish.rs
+++ b/tests/testsuite/publish.rs
@@ -130,6 +130,37 @@ You may press ctrl-c to skip waiting; the crate should be available shortly.
     validate_upload_foo();
 }
 
+#[cargo_test]
+fn duplicate_version() {
+    let registry_dupl = RegistryBuilder::new().http_api().http_index().build();
+    Package::new("foo", "0.0.1").publish();
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.0.1"
+                authors = []
+                license = "MIT"
+                description = "foo"
+            "#,
+        )
+        .file("src/main.rs", "fn main() {}")
+        .build();
+
+    p.cargo("publish")
+        .replace_crates_io(registry_dupl.index_url())
+        .with_status(101)
+        .with_stderr_data(str![[r#"
+[UPDATING] crates.io index
+[ERROR] crate foo@0.0.1 already exists on crates.io index
+
+"#]])
+        .run();
+}
+
 // Check that the `token` key works at the root instead of under a
 // `[registry]` table.
 #[cargo_test]
@@ -3848,22 +3879,7 @@ You may press ctrl-c to skip waiting; the crate should be available shortly.
         .with_status(101)
         .with_stderr_data(str![[r#"
 [UPDATING] crates.io index
-[PACKAGING] a v0.0.1 ([ROOT]/foo/a)
-[PACKAGED] 3 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
-[PACKAGING] b v0.0.1 ([ROOT]/foo/b)
-[PACKAGED] 3 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
-[VERIFYING] a v0.0.1 ([ROOT]/foo/a)
-[COMPILING] a v0.0.1 ([ROOT]/foo/target/package/a-0.0.1)
-[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
-[VERIFYING] b v0.0.1 ([ROOT]/foo/b)
-[UPDATING] crates.io index
-[ERROR] failed to verify package tarball
-
-Caused by:
-  failed to get `a` as a dependency of package `b v0.0.1 ([ROOT]/foo/target/package/b-0.0.1)`
-
-Caused by:
-  found a package in the remote registry and the local overlay: a@0.0.1
+[ERROR] crate a@0.0.1 already exists on crates.io index
 
 "#]])
         .run();
diff --git a/tests/testsuite/registry_auth.rs b/tests/testsuite/registry_auth.rs
index d73d041b5..707761cc4 100644
--- a/tests/testsuite/registry_auth.rs
+++ b/tests/testsuite/registry_auth.rs
@@ -565,9 +565,10 @@ fn token_not_logged() {
     // 2. config.json again for verification
     // 3. /index/3/b/bar
     // 4. /dl/bar/1.0.0/download
-    // 5. /api/v1/crates/new
-    // 6. config.json for the "wait for publish"
-    // 7. /index/3/f/foo for the "wait for publish"
-    assert_eq!(authorizations.len(), 7);
+    // 5. /index/3/f/foo for checking duplicate version
+    // 6. /api/v1/crates/new
+    // 7. config.json for the "wait for publish"
+    // 8. /index/3/f/foo for the "wait for publish"
+    assert_eq!(authorizations.len(), 8);
     assert!(!log.contains("a-unique_token"));
 }