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.

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

feat: Added warning when failing to update index cache #15014

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

ranger-ross
Copy link
Contributor

@ranger-ross ranger-ross commented Jan 4, 2025

What does this PR try to resolve?

Fixes #13712

Adds a warning if there is an error updating the index cache.
It also attempts to avoid warning spam as requested in this comment

Below is an example output

    Updating crates.io index
warning: failed to write cache, path: /home/ross/.cargo/registry/index/index.crates.io-1949cf8c6b5b557f/.cache/ba/se/base64, error: Permission denied (os error 13)
   Compiling serde v1.0.217
   Compiling r-efi v5.2.0
   Compiling base64 v0.22.1
   Compiling cargo-13712 v0.1.0 (/home/ross/projects/cargo-13712)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.20s

How should we test and review this PR?

I tested this on my machine by manually changing the owner/permission of the index files

sudo chown root ~/.cargo/registry/index/.../.cache/r-
sudo chmod 700 ~/.cargo/registry/index/.../.cache/r-

# in a project that uses the `r-efi` crate
cargo build 

I wasn't quiet sure about the best way to add a test to the testsuite for this. 😅
If there is good way to create a test for this lmk

@rustbot
Copy link
Collaborator

rustbot commented Jan 4, 2025

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-registries Area: registries S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 4, 2025
src/cargo/sources/registry/index/cache.rs Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

We have a similar test, maybe we can take reuse that test to assert their stderr?

fn readonly_registry_still_works() {
Package::new("foo", "0.1.0").publish();
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "a"
version = "0.5.0"
edition = "2015"
authors = []
[dependencies]
foo = '0.1.0'
"#,
)
.file("src/lib.rs", "")
.build();
p.cargo("generate-lockfile").run();
p.cargo("fetch --locked").run();
chmod_readonly(&paths::home(), true);
p.cargo("check").run();
// make sure we un-readonly the files afterwards so "cargo clean" can remove them (#6934)
chmod_readonly(&paths::home(), false);
fn chmod_readonly(path: &Path, readonly: bool) {
for entry in t!(path.read_dir()) {
let entry = t!(entry);
let path = entry.path();
if t!(entry.file_type()).is_dir() {
chmod_readonly(&path, readonly);
} else {
set_readonly(&path, readonly);
}
}
set_readonly(path, readonly);
}
fn set_readonly(path: &Path, readonly: bool) {
let mut perms = t!(path.metadata()).permissions();
perms.set_readonly(readonly);
t!(fs::set_permissions(path, perms));
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I was able to use this to create a test based off of this in 9f2d0a3

Copy link
Member

Choose a reason for hiding this comment

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

Shameless copied from #15026 (comment).

We generally ask that PRs are split into

  • One commit that adds the test, showing the currently broken behavior (ie it passes)
  • The fix commit that updates the test to pass with the change

This makes the diff between the two commits show the change in behavior

Mind doing that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I re-ordered the commits so that the test was added before the fix.
Let me know if I did something incorrectly

Copy link
Member

Choose a reason for hiding this comment

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

My original idea is that both commits should pass CI. And the second commit contains changes in both the logic fix and the test, so the test change could reflect how the behavior has changed with the fix. See this PR as an example #15036. Anyway it doesn't really matter, and thank you!

@ranger-ross ranger-ross force-pushed the cache-warnings branch 2 times, most recently from 9f2d0a3 to 92ed0f1 Compare January 8, 2025 15:04
tests/testsuite/registry.rs Outdated Show resolved Hide resolved
src/cargo/sources/registry/index/cache.rs Outdated Show resolved Hide resolved
authors = []

[dependencies]
foo = '0.1.0'
Copy link
Member

Choose a reason for hiding this comment

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

Should we add more than one dependency, so we know it really emits only once?

Copy link
Contributor Author

@ranger-ross ranger-ross Jan 9, 2025

Choose a reason for hiding this comment

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

I was testing with this and notice the dependencies would get compiled in different orders causing .with_stderr_data() to fail sometimes.

But I think this can be worked around by using the [..] to ignore the names since they are not the point of this test.

    Package::new("foo", "0.1.0").publish();
    Package::new("fo2", "0.1.0").publish();

// ...

        .with_stderr_data(str![[r#"
[WARNING] failed to write cache, path: [ROOT]/home/.cargo/registry/index/-[HASH]/.cache/3/f/fo[..], [ERROR] Permission denied (os error 13)
[COMPILING] fo[..] v0.1.0
[COMPILING] fo[..] v0.1.0
[COMPILING] a v0.5.0 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s

"#]])

I think something like this should.
But let me know if you are aware of a smarter way to do this. 👍

Copy link
Member

Choose a reason for hiding this comment

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

We do have the unordered method to relax the snapshot comparsion a bit, though your approach is pretty great!

@ranger-ross ranger-ross force-pushed the cache-warnings branch 2 times, most recently from 4d02233 to aa059f8 Compare January 10, 2025 14:12
@weihanglo weihanglo added this pull request to the merge queue Jan 10, 2025
Merged via the queue into rust-lang:master with commit c518f22 Jan 10, 2025
21 checks passed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 11, 2025
Update cargo

18 commits in fd784878cfa843e3e29a6654ecf564c62fae6735..088d496082726091024f1689c124a0c3dccbd775
2025-01-03 20:06:26 +0000 to 2025-01-10 20:10:21 +0000
- docs(reference): Fix PkgIdSpec kind docs (rust-lang/cargo#15049)
- feat: Added warning when failing to update index cache (rust-lang/cargo#15014)
- docs(ref): Fix the inverted logic about MSRV (rust-lang/cargo#15044)
- chore(deps): update msrv (1 version) to v1.84 (rust-lang/cargo#15041)
- Remove unnecessary into conversions (rust-lang/cargo#15042)
- docs(contrib): Start guidelines for schema design (rust-lang/cargo#15037)
- fix: emit warnings as warnings when learning rust target info (rust-lang/cargo#15036)
- fix(schemas): Fix the `[lints]` JSON Schema (rust-lang/cargo#15035)
- fix(schemas): Fix 'metadata' JSON Schema (rust-lang/cargo#15033)
- shorten comment on Ord for SourceKind (rust-lang/cargo#15029)
- Make `"C"` explicit in `extern "C"`. (rust-lang/cargo#15034)
- simplify SourceID Ord/Eq (rust-lang/cargo#14980)
- Setup cargo environment for `cargo rustc --print` (rust-lang/cargo#15026)
- Avoid naming variables `str` (rust-lang/cargo#15025)
- Bump to 0.87.0; update changelog (rust-lang/cargo#15022)
- Update libgit2 to 1.9 (rust-lang/cargo#15018)
- Remove condition on RUSTUP_WINDOWS_PATH_ADD_BIN (rust-lang/cargo#15017)
- Fix https::self_signed_should_fail for macos (rust-lang/cargo#15016)
@rustbot rustbot added this to the 1.86.0 milestone Jan 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-registries Area: registries S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cargo updates registry index on every command with bad permissions
3 participants