Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Bail publish job before packaging and upload (passing tests) #14338

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cargo/ops/registry/#.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
16 changes: 7 additions & 9 deletions src/cargo/ops/registry/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,14 +118,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");
Expand All @@ -134,9 +134,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()
Expand Down Expand Up @@ -168,11 +168,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,
))
}

Expand Down
2 changes: 1 addition & 1 deletion src/cargo/ops/registry/owner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
31 changes: 30 additions & 1 deletion src/cargo/ops/registry/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ use crate::ops;
use crate::ops::PackageOpts;
use crate::ops::Packages;
use crate::sources::source::QueryKind;
use crate::sources::source::Source;
use crate::sources::SourceConfigMap;
use crate::sources::CRATES_IO_REGISTRY;
use crate::util::auth;
Expand Down Expand Up @@ -129,7 +130,7 @@ pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> {
val => val,
};
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),
Expand All @@ -139,6 +140,34 @@ pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> {
)?;
verify_dependencies(pkg, &registry, source_ids.original)?;

// Bail before packaging and uploading if same version already exists in the registry

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

let _lock = opts
.gctx
.acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?;

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()?,
}
};

drop(_lock);

if !duplicate_query.is_empty() {
bail!(
"crate {}@{} already exists on {}",
pkg.name(),
pkg.version(),
source.describe()
);
}

// Prepare a tarball, with a non-suppressible warning if metadata
// is missing since this is being put online.
let tarball = ops::package_one(
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/ops/registry/search.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/ops/registry/yank.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
27 changes: 26 additions & 1 deletion tests/testsuite/credential_process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{"v":1,"registry":{"index-url":"[..]","name":"alternative"},"kind":"get","operation":"publish","name":"foo","vers":"0.1.1","cksum":"[..]"}
[UPLOADING] foo v0.1.1 ([ROOT]/foo)
[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!(
Expand All @@ -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();
}

Expand Down
35 changes: 35 additions & 0 deletions tests/testsuite/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,41 @@ 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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update the original test with these changes?


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())
.without_status()
.run();

p.cargo("publish")
.replace_crates_io(registry_dupl.index_url())
.with_stderr_data(str![[r#"
[UPDATING] crates.io index
[ERROR] crate foo@0.0.1 already exists on [..]

"#]])
.with_status(101)
.run();
}

// Check that the `token` key works at the root instead of under a
// `[registry]` table.
#[cargo_test]
Expand Down
9 changes: 5 additions & 4 deletions tests/testsuite/registry_auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Comment on lines +568 to +572
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be squashed into the appropriate commit? Ideally all commits pass tests

assert!(!log.contains("a-unique_token"));
}