Skip to content

Make CARGO_HOME optionally writable #93

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

skius
Copy link

@skius skius commented Feb 19, 2025

We're running into issues with rustwide in qrates when incremental compilation is enabled:

--snip--
14:08:57 [INFO] [stdout] 948fb9cd0c36f8264ca53a8d474817f66d2eb5db6ca5eee8a9c271c2887d41c4
14:08:57 [INFO] running `Command { std: "docker" "start" "-a" "948fb9cd0c36f8264ca53a8d474817f66d2eb5db6ca5eee8a9c271c2887d41c4", kill_on_drop: false }`
14:08:57 [INFO] [stderr]     Checking powerfmt v0.2.0
14:08:57 [INFO] [stderr]     Checking num-conv v0.1.0
14:08:57 [INFO] [stderr]     Checking time-core v0.1.2
14:08:57 [INFO] [stderr] error: could not create incremental compilation crate directory `incremental/powerfmt-33plc6te6e1kj`: Read-only file system (os error 30)
14:08:57 [INFO] [stderr]
14:08:57 [INFO] [stderr] error: could not create incremental compilation crate directory `incremental/time_core-0itg8lzr2ib0v`: Read-only file system (os error 30)
14:08:57 [INFO] [stderr]
14:08:57 [INFO] [stderr] error: could not create incremental compilation crate directory `incremental/num_conv-2hmvxbsyjvjzr`: Read-only file system (os error 30)
14:08:57 [INFO] [stderr]
14:08:57 [INFO] [stderr] error: could not compile `time-core` (lib) due to 1 previous error
14:08:57 [INFO] [stderr] warning: build failed, waiting for other jobs to finish...
14:08:57 [INFO] [stderr] error: could not compile `powerfmt` (lib) due to 1 previous error
14:08:58 [INFO] [stderr] error: could not compile `num-conv` (lib) due to 1 previous error
--snip--

This PR adds an option to Command to allow mounting the $CARGO_HOME directory as read/write, which resolves our issue.

I am unsure if this is the way to solve the issue, so I'm opening this PR in draft state. I would be happy as well if there was a way to set the incremental directory to something ephemeral inside the vfs and keeping $CARGO_HOME read-only. Our project is only interested in some side-effects of incremental compilation in rustc (computing HIR hashes), not the incremental nature itself.

Thanks!

@skius
Copy link
Author

skius commented Feb 19, 2025

The CI failure seems unrelated:

 error: a method with this name may be added to the standard library in the future
  --> src/utils.rs:45:18
   |
45 |     let _ = file.unlock();
   |                  ^^^^^^
   |
   = warning: once this associated item is added to the standard library, the ambiguity may cause an error or change in behavior!
   = note: for more information, see issue #48919 <https://github.com/rust-lang/rust/issues/48919>
   = help: call with fully qualified syntax `fs2::FileExt::unlock(...)` to keep using the current method
   = note: `-D unstable-name-collisions` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(unstable_name_collisions)]`

error: could not compile `rustwide` (lib) due to 1 previous error

@skius skius marked this pull request as ready for review March 11, 2025 14:57
@syphar syphar self-requested a review May 19, 2025 05:41
@syphar
Copy link
Member

syphar commented May 19, 2025

I wasn't aware of more users of rustwide, cool!

I totally forgot about this PR, generally don't hesitate to ping me if that happens again.
Will try to find some time to do a first review this week.

On first sight I definitely see a security impact where the build of one crate could impact the build of the next crates by just replacing the toolchain binaries or parts of the cache. The risk I see is that unaware users of this library might enable this feature without knowing what they expose themselves to

@syphar
Copy link
Member

syphar commented May 19, 2025

Since this PR is a little older: is this still something you need? @skius

@skius
Copy link
Author

skius commented May 20, 2025

The project is on hiatus for now, but it does still depend on my changes. I agree with your concerns wrt security/reproducibility impact. Perhaps defining an override for the incremental compilation directory per crate and then only making that r/w is a better alternative? Assuming that either a) single subdirectories of a r/o docker mount can be made r/w or b) we can change the incremental directory to be outside the mount, either in a separate, per-crate r/w mount, or just inside the docker fs itself?

Writing this out, for our purposes just using the ephemeral docker fs might be a good approach, since we're only interested in side-effects of incremental compilation, and are doing full builds anyway. I'd have to check if the incremental build directory can be set to something outside of $CARGO_HOME

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants