'cargo metadata' fails on read-only file system

RalfJung commented 3 years ago

Problem

I first reported this with rust-analyzer at https://github.com/rust-analyzer/rust-analyzer/issues/10792: when running cargo metadata on a read-only file system, it fails saying

rust-analyzer failed to load workspace: Failed to read Cargo metadata for Rust sources: Failed to run `cargo metadata --manifest-path /home/r/.rustup/toolchains/miri/lib/rustlib/rustc-src/rust/compiler/rustc/Cargo.toml`: `cargo metadata` exited with an error:     Updating crates.io index
error: failed to write /home/r/.rustup/toolchains/miri/lib/rustlib/rustc-src/rust/compiler/rustc/Cargo.lock

Caused by:
  failed to open: /home/r/.rustup/toolchains/miri/lib/rustlib/rustc-src/rust/compiler/rustc/Cargo.lock

Caused by:
  Read-only file system (os error 30)
rust-analyzer failed to load workspace: Failed to read Cargo metadata for Rust sources: Failed to run `cargo metadata --manifest-path /home/r/.rustup/toolchains/miri/lib/rustlib/rustc-src/rust/compiler/rustc/Cargo.toml`: `cargo metadata` exited with an error:     Updating crates.io index
error: failed to write /home/r/.rustup/toolchains/miri/lib/rustlib/rustc-src/rust/compiler/rustc/Cargo.lock

Caused by:
  failed to open: /home/r/.rustup/toolchains/miri/lib/rustlib/rustc-src/rust/compiler/rustc/Cargo.lock

Caused by:
  Read-only file system (os error 30)

However, the action RA is trying to perform is fundamentally a read-only operation (querying for information about the rustc compiler crates).

I don't think the currently available flags provide any way to make this work on a read-only file system either (I tried --frozen but as expected it refuses to download required crates from the network). Even if my toolchain was on a read-write file system I would not want RA's cargo metadata to change toolchain files, after all.

Proposed Solution

cargo metadata should either handle read-only file systems by falling back to not creating/updating the lock file -- or (IMO the right fix) it should not even attempt to mutate the workspace when what it is doing is a read-only operation like just determining the metadata.

Notes

No response

ehuss commented 3 years ago

Can you say more about your use case about why the Cargo.lock is out of date? You wouldn't be able to run any other cargo command, either, so just allowing cargo metadata to work with an out-of-date lockfile may not help much.

bjorn3 commented 3 years ago

The rustc-src rustup component doesn't have the root Cargo.toml so the root Cargo.lock isn't used. Rust-analyzer runs cargo metadata for the compiler/rustc crate which normally doesn't need it's own Cargo.lock as it uses the workspace's version, but because the root Cargo.toml is missing, this isn't possible.

RalfJung commented 3 years ago

You wouldn't be able to run any other cargo command, either, so just allowing cargo metadata to work with an out-of-date lockfile may not help much.

AFAIK cargo metadata is the only command that needs to work for RA to be able to proceed here -- but I am not sure and I wouldn't know how to test this.

aos commented 2 years ago

Hi all - I ran into this issue while loading stdlib from a read-only location (attempting to access it from Nix store). I dug into this a bit and we can trace it down to resolve_with_registry writing the lock file: https://github.com/rust-lang/cargo/blob/b816d82ee94acd4d6be588c0f0cd299701c55457/src/cargo/ops/resolve.rs#L206-L208

Since we don't write the lock file if the workspace is already marked as ephemeral, can we make the assumption that a read-only file system is inherently "ephemeral" and mark the workspace as such when creating it? The only other place where we check for is_ephemeral is when preloading a provided registry here: https://github.com/rust-lang/cargo/blob/b816d82ee94acd4d6be588c0f0cd299701c55457/src/cargo/core/workspace.rs#L1029-L1037

Perhaps the metadata command can be instantiated with an ephemeral workspace? Or checking to see if the manifest_path is read-only, and then doing some operation to not allow the lock file to be written. Unfortunately, this is my only usage of this command, so I don't know other use cases. Either way, I'm happy to work on this one!

bjorn3 commented 2 years ago

I don't think a read-only fs ahould be considered ephemeral. It can be compiled (or get metadata retrieved) twice in whoch case the same dependency versions should be used. I see rust-analyzer only reloading cargo metadata when Cargo.* changes as an optimization. cargo metadata should be an idempotent operation. That is running it twice will result in the same result. If there is no Cargo.lock that will not be true. The real fix would be to ship a correct Cargo.lock file as part of the rustc-src component.

aos commented 2 years ago

That makes sense - thanks for the insight.

RalfJung commented 2 years ago

I would say even more important than being idempotent is it being a read-only operation. It is merely querying some information and state from a crate. If that information depends on things like the crate registry, I am not surprised by multiple queries returning different results. (I would not expect curl to be idempotent, either.) However, I am severely surprised by such a command changing the on-disk state of my project, and even more surprised when there is no fallback for when performing that unexpected mutation fails.

leoluk commented 2 years ago

I would say even more important than being idempotent is it being a read-only operation.

Most cargo commands will mutate the lockfile by default - you need to specify --frozen or --locked to prevent that.

RalfJung commented 2 years ago

Yeah but those will just fail in that case, so that is not really helpful either.

I honestly don't think there is a good reason that a query like cargo metadata requires read-write access to the project files, no matter whether the lockfile is somehow out-of-date or not. Sure we could work around this issue by shipping a slightly different lockfile, but IMO that's a hack, no a solution.

bjorn3 commented 2 years ago

Sure we could work around this issue by shipping a slightly different lockfile, but IMO that's a hack, no a solution.

The problem is that the lockfile is outdated. It is the solution. Ignoring lockfile write errors is a hack IMO. cargo metadata --frozen is the only mode of operation I would expect to be entirely side effect free. Anything else may need to download crates from crates.io and store them in ~/.cargo. In addition cargo build is conceptually just cargo metadata followed by deriving a list of build commands from the result. If cargo metadata didn't write the lockfile, cargo build wouldn't either.

RalfJung commented 2 years ago

In addition cargo build is conceptually just cargo metadata followed by deriving a list of build commands from the result. If cargo metadata didn't write the lockfile, cargo build wouldn't either.

I see no good reason why it has to be like that. It certainly does not match my mental model at all.

cargo build has good reasons to fail when called on a crate stored on a read-only FS. cargo metadata IMO does not.

Anything else may need to download crates from crates.io and store them in ~/.cargo.

Populating a cache is (conceptually) not a side-effect (formally speaking: a memoized function is observably pure). So I don't mind it changing ~/.cargo. That is categorically different from mutating the crate it is working on.

RalfJung commented 1 year ago

Not sure if that is related, but there are also cases where cargo metadata fails because it cannot update the index, but doing a build actually works fine -- see https://github.com/rust-lang/rust-analyzer/issues/12499.

gilescope commented 1 year ago

It seems that this can trigger unnecessary rebuilds if a rerun_if_changed is issued on Cargo.lock.

RalfJung commented 1 year ago

Unfortunately it seems that even if a lockfile exists that has all the required information to make the metadata call idempotent, cargo will still insist on writing to that lockfile if the lockfile also contains information for other creates that are not present in the workspace. Even if one buys into the idempotency argument (which I don't), that should still be considered a bug.

This bug means that just copying the lockfile from lib/rustlib/rustc-src/rust/Cargo.lock to lib/rustlib/rustc-src/rust/compiler/rustc/Cargo.lock is not enough to make RA work with a read-only rustup directory.

GenericNerdyUsername commented 7 months ago

This seems to have stalled and I need it to be fixed. Whats needed for progress?

epage commented 7 months ago

@GenericNerdyUsername generally a good first step is to summarize the existing discussion and the different angles.

For example, this thread has people with a couple different perspectives

cargo metadata should be consistent in its output which can only happen if we can write a Cargo.lock to the filesystem
cargo metadata should always let me query information, even it means the information changes due to outside changes (e.g. a new version of a dependency is released so it gets selected)

So not much so far. Now, putting on my cargo hat, the problem I see with the latter is user intention. Today, you can run cargo metadata --no-deps and no Cargo.lock gets generated. Once you ask for dependencies, you are asking for dependencies of that instance of the project and not some ephemeral state. There might be cases, like in this thread, where something else is needed, but that likely runs counter to other uses. To move this forward, we'd need to find a way to balance these two needs.

Some potential ideas

If we had #5707, then the caller can decide that they don't care about the exact state of the dependencies, check if its read-only, and specify a lockfile from a temp location. In this case, a caller like rust-analyzer could even make that temp location keyed based off of the project so it stays consistent
With #5221 and cargo script, we've talked about the idea of locking to timestamps, rather than to specific dependencies. This is a lot more radical idea with a lot of steps in the middle to be figured out.
If a workaround is to allow reusing lockfiles between projects, then we could limit what project information we put in (e.g. #5979) and make it so we skip writing the lockfile if the one on-disk is a superset of what we've resolved. A problem with that is removing dependencies won't change your lockfile which might surprise and upset people (particularly if they are relying on the lockfile for audits). Maybe we could check if the lockfile is a superset of what was resolved and skip the write if it errored?

GenericNerdyUsername commented 7 months ago

the solution seems pretty clear to me, if the user wants idempotent behavior but readonly is implemented, they can just update the lockfile beforehand. If the user wants RO behavior but idempotent is whats implemented, they're screwed (afaict).

epage commented 7 months ago

Another potential way of addressing this is error recovery. We report all errors that happened along the way and we report the metadata result (with a bad exit code).

However, if someone wants to only ignore read-only file systems but error for the rest (like bad toml syntax), it might be difficult.

GenericNerdyUsername commented 7 months ago

What would be the downsides of not writing to the lockfile, other than needing another command beforehand if you want consistent output?

GenericNerdyUsername commented 5 months ago

any change on this?

Ifropc commented 4 months ago

Just my 5 cents: For the mentioned above cases, I believe the first mentioned option (#5707) would be the best solution. It feels like the easiest approach to write something like a lock file on a modifiable FS while keeping original folder read-only with underlying .toml file. The caller in this case should be responsible for managing the lockfile. (it's also possible to add a root directory storing lockfiles in Cargo config file in the future) As for the #5979 suggestion, I would imagine the issue would still happen if the user would want to index a freshly downloaded nightly on a read-only FS (as no superset lockfile would exists), or if the user simply has a fresh install on read-only FS. (Please correct me if I'm wrong) And as mentioned before, #5221 could be a bit too complex in a scope of this issue.

That being said, it looks like there was an attempt to make an alternative solution with introducing a new flag in #10716 which was previously rejected.

I'm not sure that is something that would warrant supporting or adding a new flag for, at least not without significant justification.

@epage do you know by any change, if let's say #5707 was implemented, this issue is a strong enough argument for adding a new flag? I'd imagine the team wouldn't want to add new flags on a whim, but this issue seem to be impactful enough (e.g. people are experiencing Jetbrains IDEs such as rust-rover not working on read-only installations of rust)

I'd love to take on #5707 implementation (as it seems to not be too complicated, at least at the moment), but as a first-time committer would be happy to hear opinion of someone's experienced.

GenericNerdyUsername commented 4 months ago

you could implement the ro mechanism without adding another flag by adding a special case for --lockfile-path

Ifropc commented 4 months ago

you could implement the ro mechanism without adding another flag by adding a special case for --lockfile-path

I think as long lockfile is written in another directory, original toml path can be RO (assuming we adding a --lockfile-path). I don't think any other flag would be necessary.

epage commented 4 months ago

We discussed #5707 in today's Cargo team meeting and are good with it moving forward. I've marked it as "Accepted" and laid out some high level information. I would recommend checking out https://doc.crates.io/contrib/ for more details on processes (both PR processes and "how to add unstable features")

Ifropc commented 4 months ago

Thanks for the follow-up and providing the design @epage! I'm going to scope out the work to be done and follow up with the questions (if any) in the #5707 Let's keep the conversation there and I'll try to get the implementation out ASAP as this issue been lingering for a while

Ifropc commented 3 months ago

FYI: lockfile path PR has been merged. We can now call cargo metadata --lockfile-path /my/write/path/Cargo.lock @GenericNerdyUsername @RalfJung you might be interested in it ^

weihanglo commented 3 months ago

Just FYI, it will be available in next one or two nightly release https://github.com/rust-lang/rust/pull/129178.

RalfJung commented 3 months ago

Awesome, thanks. :)

I guess now the question is whether RA is interested in using this feature to support running RA in situations where the sysroot is in a read-only location. I reopened https://github.com/rust-lang/rust-analyzer/issues/10792 for that.

rust-lang / cargo