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

Re-enable compatibility with readonly CARGO_HOME #6940

Merged
merged 1 commit into from
May 14, 2019

Conversation

alexcrichton
Copy link
Member

Previously Cargo would attempt to work as much as possible with a
previously filled out CARGO_HOME, even if it was mounted as read-only.
In #6880 this was regressed as a few global locks and files were always
attempted to be opened in writable mode.

This commit fixes these issues by correcting two locations:

  • First the global package cache lock has error handling to allow
    acquiring the lock in read-only mode inaddition to read/write mode. If
    the read/write mode failed due to an error that looks like a readonly
    filesystem then we assume everything in the package cache is readonly
    and we switch to just acquiring any lock, this time a shared readonly
    one. We in theory aren't actually doing any synchronization at that
    point since it's all readonly anyway.

  • Next when unpacking package we're careful to issue a stat call
    before opening a file in writable mode. This way our preexisting guard
    to return early if a package is unpacked will succeed before we open
    anything in writable mode.

Closes #6928

@rust-highfive
Copy link

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 14, 2019
@alexcrichton
Copy link
Member Author

For posterity, the way I tested this was:

$ cargo build --features vendored-openssl
$ docker run \
  -v `rustc --print sysroot`:/rust:ro \
  -e RUSTC=/rust/bin/rustc \
  -w /src \
  -it \
  -v `pwd`:/src \
  -v $HOME/.cargo:/cargo:ro \
  -e CARGO_HOME=/cargo \
  ubuntu:18.04 \
  ./target/debug/cargo build

If that progresses to the point that it fails the build due to a missing cc then it means we got past the resolution phase and we've actually started executing rustc and everything is downloaded. I hit the two errors fixed here previously and afterwards it made more progress.

I'm not entirely certain if this matches up with Crater's use case, so I'm not 100% certain it will solve it.

@alexcrichton
Copy link
Member Author

Oh right there's these things called permission bits, let me add a test.

Previously Cargo would attempt to work as much as possible with a
previously filled out CARGO_HOME, even if it was mounted as read-only.
In rust-lang#6880 this was regressed as a few global locks and files were always
attempted to be opened in writable mode.

This commit fixes these issues by correcting two locations:

* First the global package cache lock has error handling to allow
  acquiring the lock in read-only mode inaddition to read/write mode. If
  the read/write mode failed due to an error that looks like a readonly
  filesystem then we assume everything in the package cache is readonly
  and we switch to just acquiring any lock, this time a shared readonly
  one. We in theory aren't actually doing any synchronization at that
  point since it's all readonly anyway.

* Next when unpacking package we're careful to issue a `stat` call
  before opening a file in writable mode. This way our preexisting guard
  to return early if a package is unpacked will succeed before we open
  anything in writable mode.

Closes rust-lang#6928
@alexcrichton
Copy link
Member Author

r? @ehuss

@rust-highfive rust-highfive assigned ehuss and unassigned nrc May 14, 2019
@ehuss
Copy link
Contributor

ehuss commented May 14, 2019

@bors r+

@bors
Copy link
Contributor

bors commented May 14, 2019

📌 Commit 5d9383e has been approved by ehuss

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 14, 2019
@bors
Copy link
Contributor

bors commented May 14, 2019

⌛ Testing commit 5d9383e with merge 414c1eb...

bors added a commit that referenced this pull request May 14, 2019
Re-enable compatibility with readonly CARGO_HOME

Previously Cargo would attempt to work as much as possible with a
previously filled out CARGO_HOME, even if it was mounted as read-only.
In #6880 this was regressed as a few global locks and files were always
attempted to be opened in writable mode.

This commit fixes these issues by correcting two locations:

* First the global package cache lock has error handling to allow
  acquiring the lock in read-only mode inaddition to read/write mode. If
  the read/write mode failed due to an error that looks like a readonly
  filesystem then we assume everything in the package cache is readonly
  and we switch to just acquiring any lock, this time a shared readonly
  one. We in theory aren't actually doing any synchronization at that
  point since it's all readonly anyway.

* Next when unpacking package we're careful to issue a `stat` call
  before opening a file in writable mode. This way our preexisting guard
  to return early if a package is unpacked will succeed before we open
  anything in writable mode.

Closes #6928
@bors
Copy link
Contributor

bors commented May 14, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: ehuss
Pushing 414c1eb to master...

@bors bors merged commit 5d9383e into rust-lang:master May 14, 2019
bors added a commit to rust-lang/rust that referenced this pull request May 16, 2019
Update cargo

17 commits in 759b6161a328db1d4863139e90875308ecd25a75..c4fcfb725b4be00c72eb9cf30c7d8b095577c280
2019-05-06 20:47:49 +0000 to 2019-05-15 19:48:47 +0000
- tests: registry: revert readonly permission after running tests. (rust-lang/cargo#6947)
- Remove Candidate (rust-lang/cargo#6946)
- Fix for "Running cargo update without a Cargo.lock ignores arguments" rust-lang/cargo#6872 (rust-lang/cargo#6904)
- Fix a minor mistake in the changelog. (rust-lang/cargo#6944)
- Give a better error message when crates.io requests time out (rust-lang/cargo#6936)
- Re-enable compatibility with readonly CARGO_HOME (rust-lang/cargo#6940)
- Fix version of `ignore`. (rust-lang/cargo#6938)
- Stabilize offline mode. (rust-lang/cargo#6934)
- zsh: Add doc options to include non-public items documentation (rust-lang/cargo#6929)
- zsh: Suggest --lib option as binary template now the default (rust-lang/cargo#6926)
- Migrate package include/exclude to gitignore patterns. (rust-lang/cargo#6924)
- Implement the Cargo half of pipelined compilation (take 2) (rust-lang/cargo#6883)
- Always include `Cargo.toml` when packaging. (rust-lang/cargo#6925)
- Remove unnecessary calls to masquerade_as_nightly_cargo. (rust-lang/cargo#6923)
- download: fix "Downloaded 1 crates" message (crates -> crate) (rust-lang/cargo#6920)
- Changed RUST_LOG usage to CARGO_LOG to avoid confusion. (rust-lang/cargo#6918)
- crate download: don't print that a crate was the largest download if it was the only download (rust-lang/cargo#6916)
@alexcrichton alexcrichton deleted the readonly-compat branch June 5, 2019 19:43
@djahandarie
Copy link

djahandarie commented Jul 11, 2019

Is this PR a full solution to the issue given NixOS/nixpkgs#61618?

alexcrichton added a commit to alexcrichton/cargo that referenced this pull request Jul 19, 2019
This commit updates support from rust-lang#6940 to not only gracefully handle
situations where the lock can be acquired in readonly but not read/write
mode but also handle situations where even a readonly lock can't be
acquired. If a readonly lock can't be acquired (and the read/write
failed) then we likely can't touch anything in the directory, so there's
no value gained from locking anyway.

Closes rust-lang#7147
bors added a commit that referenced this pull request Jul 19, 2019
Don't fail if we can't acquire readonly lock

This commit updates support from #6940 to not only gracefully handle
situations where the lock can be acquired in readonly but not read/write
mode but also handle situations where even a readonly lock can't be
acquired. If a readonly lock can't be acquired (and the read/write
failed) then we likely can't touch anything in the directory, so there's
no value gained from locking anyway.

Closes #7147
ehuss pushed a commit to ehuss/cargo that referenced this pull request Jul 23, 2019
This commit updates support from rust-lang#6940 to not only gracefully handle
situations where the lock can be acquired in readonly but not read/write
mode but also handle situations where even a readonly lock can't be
acquired. If a readonly lock can't be acquired (and the read/write
failed) then we likely can't touch anything in the directory, so there's
no value gained from locking anyway.

Closes rust-lang#7147
@jmrgibson
Copy link

Does this mean the cache should work in read-only mode too? We've got rust builds running in CI/CD, we'd like the rust toolchain directory and cargo binaries (eg cargo, fmt, etc) to be read-only for the service account running the builds, but at the moment that gets messed up because the cache directory is the same as CARGO_HOME.

@ehuss ehuss added this to the 1.36.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo check --frozen requires the cargo home to be writable on nightly
7 participants