Skip to content

fix(publish): Check remote git registry more than once post-publish #11255

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

Merged
merged 1 commit into from
Oct 20, 2022
Merged

fix(publish): Check remote git registry more than once post-publish #11255

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 20 additions & 15 deletions src/cargo/sources/registry/remote.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ pub struct RemoteRegistry<'cfg> {
head: Cell<Option<git2::Oid>>,
current_sha: Cell<Option<InternedString>>,
needs_update: bool, // Does this registry need to be updated?
updated: bool, // Has this registry been updated this session?
}

impl<'cfg> RemoteRegistry<'cfg> {
Expand All @@ -49,7 +48,6 @@ impl<'cfg> RemoteRegistry<'cfg> {
head: Cell::new(None),
current_sha: Cell::new(None),
needs_update: false,
updated: false,
}
}

Expand Down Expand Up @@ -141,6 +139,14 @@ impl<'cfg> RemoteRegistry<'cfg> {
self.current_sha.set(Some(sha));
Some(sha)
}

fn is_updated(&self) -> bool {
self.config.updated_sources().contains(&self.source_id)
}

fn mark_updated(&self) {
self.config.updated_sources().insert(self.source_id);
}
}

const LAST_UPDATED_FILE: &str = ".last-updated";
Expand Down Expand Up @@ -214,7 +220,7 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> {

match load_helper(&self, path, index_version) {
Ok(result) => Poll::Ready(Ok(result)),
Err(_) if !self.updated => {
Err(_) if !self.is_updated() => {
// If git returns an error and we haven't updated the repo, return
// pending to allow an update to try again.
self.needs_update = true;
Expand Down Expand Up @@ -250,19 +256,20 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> {
return Ok(());
}

self.updated = true;
self.needs_update = false;

if self.config.offline() {
// Make sure the index is only updated once per session since it is an
// expensive operation. This generally only happens when the resolver
// is run multiple times, such as during `cargo publish`.
if self.is_updated() {
return Ok(());
}
if self.config.cli_unstable().no_index_update {
self.mark_updated();

if self.config.offline() {
return Ok(());
}
// Make sure the index is only updated once per session since it is an
// expensive operation. This generally only happens when the resolver
// is run multiple times, such as during `cargo publish`.
if self.config.updated_sources().contains(&self.source_id) {
if self.config.cli_unstable().no_index_update {
return Ok(());
}

Expand Down Expand Up @@ -291,7 +298,6 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> {
let repo = self.repo.borrow_mut().unwrap();
git::fetch(repo, url.as_str(), &self.index_git_ref, self.config)
.with_context(|| format!("failed to fetch `{}`", url))?;
self.config.updated_sources().insert(self.source_id);

// Create a dummy file to record the mtime for when we updated the
// index.
Expand All @@ -301,13 +307,12 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> {
}

fn invalidate_cache(&mut self) {
if !self.updated {
self.needs_update = true;
}
// To fully invalidate, undo `mark_updated`s work
self.needs_update = true;
Comment on lines +310 to +311
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't cargo remove source from self.config.updated_sources() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arlosi thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the intent of updated_sources was to prevent fetching the repo more than once per session for scenarios like publish (where we do an update for fetching config.json as well as a later update for the verification build).

Maybe it's not a big deal to do an extra fetch here though, since we'll be doing many more fetches with the polling for publish anyway and it would simplify things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking some more about this, I feel like more investigation is needed into updated_sources before clearing it here in case there are areas that are relying on this behavior. I feel like that effort is best decoupled from the current effort.

Copy link
Member

Choose a reason for hiding this comment

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

Sound reasonable. We can deal with it afterward.

}

fn is_updated(&self) -> bool {
self.updated
self.is_updated()
}

fn download(&mut self, pkg: PackageId, checksum: &str) -> CargoResult<MaybeLock> {
Expand Down