Skip to content

Commit

Permalink
Auto merge of #11062 - epage:wait, r=weihanglo
Browse files Browse the repository at this point in the history
fix(publish): Block until it is in index

Originally, crates.io would block on publish requests until the publish
was complete, giving `cargo publish` this behavior by extension.  When
crates.io switched to asynchronous publishing, this intermittently broke
people's workflows when publishing multiple crates.  I say interittent
because it usually works until it doesn't and it is unclear why to the
end user because it will be published by the time they check.  In the
end, callers tend to either put in timeouts (and pray), poll the
server's API, or use `crates-index` crate to poll the index.

This isn't sufficient because
- For any new interested party, this is a pit of failure they'll fall
  into
- crates-index has re-implemented index support incorrectly in the past,
  currently doesn't handle auth, doesn't support `git-cli`, etc.
- None of these previous options work if we were to implement
  workspace-publish support (#1169)
- The new sparse registry might increase the publish times, making the
  delay easier to hit manually
- The new sparse registry goes through CDNs so checking the server's API
  might not be sufficient
- Once the sparse registry is available, crates-index users will find
  out when the package is ready in git but it might not be ready through
  the sparse registry because of CDNs

So now `cargo` will block until it sees the package in the index.
- This is checking via the index instead of server APIs in case there
  are propagation delays.  This has the side effect of being noisy
  because of all of the "Updating index" messages.
- This blocks by default but there is an unstable `publish.timeout` config field that will disable blocking when set to 0.  See #11222 for stablization

Blocking is opt-out as that is the less error prone case for casual users while those doing larger integrations are also likely to do the testing needed to make more complicated scenarios work where blocking is disabled.

Right now we block after the publish.  An alternative would be to block until all dependencies are in the index which makes the blocking only happen when needed
- Blocking on dependencies can be imprecise to detect when to block vs propagate an error up
- This is the less error prone case for users.  For example I recently publish a crate in one tab and immediately switched to another tab to use it and this only worked because `cargo-release` blocked until it was ready to use

In reviewing this change, be sure to look at the individual commits
- The first makes it possible to write the tests for this
- The second adds a test that shows the current behavior
- The third updates the test to the expected behavior, showing all of this works

In addition to the publish tests:
- We want to maximize the nightly-to-stable time to collect feedback
- We will put this in TWiR's testing section to raise visibility

Fixes #9507
  • Loading branch information
bors committed Oct 27, 2022
2 parents 1985caf + f2fc5ca commit 7e484fc
Show file tree
Hide file tree
Showing 8 changed files with 246 additions and 88 deletions.
2 changes: 1 addition & 1 deletion src/cargo/ops/registry.rs
Expand Up @@ -192,7 +192,7 @@ pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> {
opts.dry_run,
)?;
if !opts.dry_run {
const DEFAULT_TIMEOUT: u64 = 0;
const DEFAULT_TIMEOUT: u64 = 60;
let timeout = if opts.config.cli_unstable().publish_timeout {
let timeout: Option<u64> = opts.config.get("publish.timeout")?;
timeout.unwrap_or(DEFAULT_TIMEOUT)
Expand Down
12 changes: 12 additions & 0 deletions tests/testsuite/artifact_dep.rs
Expand Up @@ -1872,7 +1872,9 @@ fn env_vars_and_build_products_for_various_build_targets() {

#[cargo_test]
fn publish_artifact_dep() {
// HACK below allows us to use a local registry
let registry = registry::init();

Package::new("bar", "1.0.0").publish();
Package::new("baz", "1.0.0").publish();

Expand Down Expand Up @@ -1901,6 +1903,15 @@ fn publish_artifact_dep() {
.file("src/lib.rs", "")
.build();

// HACK: Inject `foo` directly into the index so `publish` won't block for it to be in
// the index.
//
// This is to ensure we can verify the Summary we post to the registry as doing so precludes
// the registry from processing the publish.
Package::new("foo", "0.1.0")
.file("src/lib.rs", "")
.publish();

p.cargo("publish -Z bindeps --no-verify")
.replace_crates_io(registry.index_url())
.masquerade_as_nightly_cargo(&["bindeps"])
Expand All @@ -1909,6 +1920,7 @@ fn publish_artifact_dep() {
[UPDATING] [..]
[PACKAGING] foo v0.1.0 [..]
[UPLOADING] foo v0.1.0 [..]
[UPDATING] [..]
",
)
.run();
Expand Down
14 changes: 13 additions & 1 deletion tests/testsuite/credential_process.rs
@@ -1,6 +1,6 @@
//! Tests for credential-process.

use cargo_test_support::registry::TestRegistry;
use cargo_test_support::registry::{Package, TestRegistry};
use cargo_test_support::{basic_manifest, cargo_process, paths, project, registry, Project};
use std::fs;

Expand Down Expand Up @@ -94,6 +94,16 @@ fn warn_both_token_and_process() {
.file("src/lib.rs", "")
.build();

// HACK: Inject `foo` directly into the index so `publish` won't block for it to be in
// the index.
//
// This is to ensure we can verify the Summary we post to the registry as doing so precludes
// the registry from processing the publish.
Package::new("foo", "0.1.0")
.file("src/lib.rs", "")
.alternative(true)
.publish();

p.cargo("publish --no-verify --registry alternative -Z credential-process")
.masquerade_as_nightly_cargo(&["credential-process"])
.with_status(101)
Expand Down Expand Up @@ -125,6 +135,7 @@ Only one of these values may be set, remove one or the other to proceed.
[UPDATING] [..]
[PACKAGING] foo v0.1.0 [..]
[UPLOADING] foo v0.1.0 [..]
[UPDATING] [..]
",
)
.run();
Expand Down Expand Up @@ -198,6 +209,7 @@ fn publish() {
[UPDATING] [..]
[PACKAGING] foo v0.1.0 [..]
[UPLOADING] foo v0.1.0 [..]
[UPDATING] [..]
",
)
.run();
Expand Down
4 changes: 3 additions & 1 deletion tests/testsuite/cross_publish.rs
Expand Up @@ -66,7 +66,8 @@ fn publish_with_target() {
return;
}

let registry = registry::init();
// `publish` generally requires a remote registry
let registry = registry::RegistryBuilder::new().http_api().build();

let p = project()
.file(
Expand Down Expand Up @@ -109,6 +110,7 @@ fn publish_with_target() {
[COMPILING] foo v0.0.0 ([CWD]/target/package/foo-0.0.0)
[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
[UPLOADING] foo v0.0.0 ([CWD])
[UPDATING] crates.io index
",
)
.run();
Expand Down
24 changes: 24 additions & 0 deletions tests/testsuite/features_namespaced.rs
Expand Up @@ -858,7 +858,9 @@ bar v1.0.0

#[cargo_test]
fn publish_no_implicit() {
// HACK below allows us to use a local registry
let registry = registry::init();

// Does not include implicit features or dep: syntax on publish.
Package::new("opt-dep1", "1.0.0").publish();
Package::new("opt-dep2", "1.0.0").publish();
Expand All @@ -885,13 +887,23 @@ fn publish_no_implicit() {
.file("src/lib.rs", "")
.build();

// HACK: Inject `foo` directly into the index so `publish` won't block for it to be in
// the index.
//
// This is to ensure we can verify the Summary we post to the registry as doing so precludes
// the registry from processing the publish.
Package::new("foo", "0.1.0")
.file("src/lib.rs", "")
.publish();

p.cargo("publish --no-verify")
.replace_crates_io(registry.index_url())
.with_stderr(
"\
[UPDATING] [..]
[PACKAGING] foo v0.1.0 [..]
[UPLOADING] foo v0.1.0 [..]
[UPDATING] [..]
",
)
.run();
Expand Down Expand Up @@ -971,7 +983,9 @@ feat = ["opt-dep1"]

#[cargo_test]
fn publish() {
// HACK below allows us to use a local registry
let registry = registry::init();

// Publish behavior with explicit dep: syntax.
Package::new("bar", "1.0.0").publish();
let p = project()
Expand All @@ -997,6 +1011,15 @@ fn publish() {
.file("src/lib.rs", "")
.build();

// HACK: Inject `foo` directly into the index so `publish` won't block for it to be in
// the index.
//
// This is to ensure we can verify the Summary we post to the registry as doing so precludes
// the registry from processing the publish.
Package::new("foo", "0.1.0")
.file("src/lib.rs", "")
.publish();

p.cargo("publish")
.replace_crates_io(registry.index_url())
.with_stderr(
Expand All @@ -1007,6 +1030,7 @@ fn publish() {
[COMPILING] foo v0.1.0 [..]
[FINISHED] [..]
[UPLOADING] foo v0.1.0 [..]
[UPDATING] [..]
",
)
.run();
Expand Down

0 comments on commit 7e484fc

Please sign in to comment.