Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Unconditionally query crates.io HTTP index for publish status #693

Closed
wants to merge 5 commits into from
Closed

Unconditionally query crates.io HTTP index for publish status #693

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
012d3e9
Replace crates-index with tame-index
Jake-Shadle Aug 22, 2023
145bb50
Change to purely use remote sparse index
Jake-Shadle Aug 23, 2023
44b023b
Return MSRV to 1.66.0
Jake-Shadle Aug 23, 2023
d3253ce
Update MSRV
Jake-Shadle Aug 23, 2023
db48047
Bump to 1.67.1
Jake-Shadle Aug 23, 2023
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
1,770 changes: 1,655 additions & 115 deletions Cargo.lock
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ vendored-openssl = ["git2/vendored-openssl"]

[dependencies]
cargo_metadata = "0.15"
crates-index = "0.19"
tame-index = { version = "0.4", features = ["git", "sparse"] }
git2 = { version = "0.17.2", default-features = false }
toml_edit = "0.19.10"
toml = "0.7.4"
Expand Down
2 changes: 1 addition & 1 deletion src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ process_error_from!(anyhow::Error);
process_error_from!(std::io::Error);
process_error_from!(semver::Error);
process_error_from!(ignore::Error);
process_error_from!(crates_index::Error);
process_error_from!(tame_index::Error);
process_error_from!(cargo_metadata::Error);
process_error_from!(toml::ser::Error);
process_error_from!(toml_edit::ser::Error);
Expand Down
29 changes: 20 additions & 9 deletions src/ops/cargo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ pub fn publish(
}

pub fn wait_for_publish(
index: &mut crates_index::Index,
index: &mut tame_index::index::ComboIndex,
name: &str,
version: &str,
timeout: std::time::Duration,
Expand All @@ -122,8 +122,10 @@ pub fn wait_for_publish(
let sleep_time = std::time::Duration::from_secs(1);
let mut logged = false;
loop {
if let Err(e) = index.update() {
log::debug!("crate index update failed with {}", e);
if let tame_index::index::ComboIndex::Git(gi) = index {
if let Err(e) = gi.fetch() {
log::debug!("crate index update failed with {}", e);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the locking scheme?

Granted, I'm not correctly handling it for crates_index today but its what has stopped me from migrating from v1 to v2 in cargo-edit is the lack of clarify on this.

In cargo-edit, we've had to implement a retry loop for locking. I see that tame-index advertises (b)locking support but its unclear to me which locks are involved seeing as tame-index is directly reading and manipulating internal cargo caches (not really thrilled with that btw). Is it using the same locking policy as cargo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the opening of a git index is locked via gix's mechanism, but tame-index doesn't currently support the global package lock mechanism that cargo uses. I'll open an issue to add that eventually, but I think the easier thing to do for this crate is to simply do one of the options I mentioned in the PR, and just only use the sparse index, and do zero disk operations at all, and remove the gix dependency.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm hesitant about the idea of exclusively doing network operations to the point that I'd need a strong justification to move this PR forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is that a problem? The current code is doing far more network I/O due to always performing a git fetch, changing it to do a single HTTP request for the crate being released with its etag means drastically reduced and faster checking for the publish status of the crate.

}
if is_published(index, name, version) {
break;
Expand All @@ -145,12 +147,21 @@ pub fn wait_for_publish(
Ok(())
}

pub fn is_published(index: &crates_index::Index, name: &str, version: &str) -> bool {
let crate_data = index.crate_(name);
crate_data
.iter()
.flat_map(|c| c.versions().iter())
.any(|v| v.version() == version)
pub fn is_published(index: &tame_index::index::ComboIndex, name: &str, version: &str) -> bool {
match index.krate(name.try_into().expect("crate name is invalid"), true) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this call always do a network operation for sparse index?

Inside of cargo, we unconditionally cache lookups in-memory (and on-disk) and it requires an explicit call to force a new lookup. If the caller has to explicitly decide to do a krate call and then cached_krate, that seems onerous.

If this doesn't always do a network operation, then this is broken for sparse index as we need to ensure we get the latest published status.
(granted, I should soon just say we don' support those older cargos)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For git it does no network I/O only reading from the cache or git blob if the cache is outdated, for sparse it always does a network call, using the local cache entry to set the e-tag so that the response will only be 200 if the crate metadata has been updated.

Ok(Some(crate_data)) => crate_data
.versions
.into_iter()
.any(|iv| iv.version == version),
Ok(None) => false,
Err(err) => {
// For both http and git indices, this _might_ be an error that goes away in
// a future call, but at least printing out something should give the user
// an indication something is amiss
log::warn!("failed to read metadata for {name}: {err:#}");
false
}
}
}

pub fn set_workspace_version(
Expand Down
2 changes: 1 addition & 1 deletion src/steps/hook.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ pub struct HookStep {
impl HookStep {
pub fn run(&self) -> Result<(), CliError> {
git::git_version()?;
let index = crates_index::Index::new_cargo_default()?;
let index = crate::steps::index::open_crates_io_index()?;

if self.dry_run {
let _ =
Expand Down
24 changes: 24 additions & 0 deletions src/steps/index.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
pub(crate) fn open_crates_io_index() -> Result<tame_index::index::ComboIndex, crate::error::CliError>
{
let index = tame_index::index::ComboIndexCache::new(tame_index::IndexLocation::new(
tame_index::IndexUrl::crates_io(None, None, None)?,
))?;

let index = match index {
tame_index::index::ComboIndexCache::Git(gi) => {
tame_index::index::RemoteGitIndex::new(gi)?.into()
}
tame_index::index::ComboIndexCache::Sparse(si) => {
tame_index::index::RemoteSparseIndex::new(
si,
tame_index::external::reqwest::blocking::Client::builder()
.http2_prior_knowledge()
.build()
.map_err(tame_index::Error::from)?,
)
.into()
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not too thrilled with the amount of ceremony from this API...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understandable, but this API was a response to crates-index having a much too rigid API that made testing and integration into other crates much more difficult, and some scenarios impossible without replicating pieces of logic that were not public in crates-index.

That being said it's trivial to to add helper functions to tame-index to open the crates.io index with zero knobs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A lot of libraries have a builder for complex operations and then have a convenience new function on the built type. I would assume something like that would exist here.


Ok(index)
}
9 changes: 7 additions & 2 deletions src/steps/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pub mod changes;
pub mod commit;
pub mod config;
pub mod hook;
mod index;
pub mod owner;
pub mod plan;
pub mod publish;
Expand Down Expand Up @@ -213,7 +214,7 @@ pub fn verify_monotonically_increasing(

pub fn verify_rate_limit(
pkgs: &[plan::PackageRelease],
index: &crates_index::Index,
index: &tame_index::index::ComboIndex,
dry_run: bool,
level: log::Level,
) -> Result<bool, crate::error::CliError> {
Expand All @@ -227,7 +228,11 @@ pub fn verify_rate_limit(
for pkg in pkgs {
if pkg.config.registry().is_none() && pkg.config.publish() {
let crate_name = pkg.meta.name.as_str();
if index.crate_(crate_name).is_some() {
if index
.krate(crate_name.try_into()?, true)
.map(|ik| ik.is_some())
.unwrap_or(false)
{
existing += 1;
} else {
new += 1;
Expand Down
4 changes: 2 additions & 2 deletions src/steps/publish.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ impl PublishStep {

let mut pkgs = plan::plan(pkgs)?;

let mut index = crates_index::Index::new_cargo_default()?;
let mut index = crate::steps::index::open_crates_io_index()?;
for pkg in pkgs.values_mut() {
if pkg.config.registry().is_none() && pkg.config.release() {
let crate_name = pkg.meta.name.as_str();
Expand Down Expand Up @@ -156,7 +156,7 @@ impl PublishStep {
pub fn publish(
ws_meta: &cargo_metadata::Metadata,
pkgs: &[plan::PackageRelease],
index: &mut crates_index::Index,
index: &mut tame_index::index::ComboIndex,
dry_run: bool,
) -> Result<(), CliError> {
for pkg in pkgs {
Expand Down
8 changes: 6 additions & 2 deletions src/steps/release.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ pub struct ReleaseStep {
impl ReleaseStep {
pub fn run(&self) -> Result<(), CliError> {
git::git_version()?;
let mut index = crates_index::Index::new_cargo_default()?;
let mut index = crate::steps::index::open_crates_io_index()?;

if self.dry_run {
let _ =
Expand All @@ -73,7 +73,11 @@ impl ReleaseStep {
pkg.bump(level_or_version, self.metadata.as_deref())?;
}
}
if index.crate_(&pkg.meta.name).is_some() {
if index
.krate(pkg.meta.name.as_str().try_into()?, true)
.map(|ik| ik.is_some())
.unwrap_or(false)
{
// Already published, skip it. Use `cargo release owner` for one-time updates
pkg.ensure_owners = false;
}
Expand Down
2 changes: 1 addition & 1 deletion src/steps/replace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ pub struct ReplaceStep {
impl ReplaceStep {
pub fn run(&self) -> Result<(), CliError> {
git::git_version()?;
let index = crates_index::Index::new_cargo_default()?;
let index = crate::steps::index::open_crates_io_index()?;

if self.dry_run {
let _ =
Expand Down