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

cargo fmt --all downloads dependencies #4599

Closed
jonhoo opened this issue Dec 16, 2020 · 4 comments
Closed

cargo fmt --all downloads dependencies #4599

jonhoo opened this issue Dec 16, 2020 · 4 comments

Comments

@jonhoo
Copy link
Contributor

jonhoo commented Dec 16, 2020

Describe the bug

When running cargo fmt with the --all argument, it will download all dependencies in the current workspace, including for targets not used on the current platform.

To Reproduce

  1. Create a new empty crate and add a dependency on serde = "1".
  2. Remove serde-*.crate from ~/.cargo/registry/cache/github.com-*/
  3. Run cargo fmt
  4. Check that no serde-*.crate files were downloaded into ~/.cargo/registry/cache/
  5. Run cargo fmt --all
  6. Observe that the call takes longer, and that serde-*.crate was downloaded into ~/.cargo/registry/cache/

Furthermore, do the above with a cfg-gated dependency on serde using

[target.'cfg(bogus)'.dependencies]
serde = "0.2"

Notice that serde is still downloaded with --all, even though it does not even meet the target platform.

Expected behavior

cargo fmt should never download dependencies, whether run with --all or not.

Meta

  • rustfmt version: rustfmt 1.4.24-stable (eb894d53 2020-11-05)
  • From where did you install rustfmt?: rustup
  • How do you run rustfmt: cargo-fmt

Diagnosis

The origin of this issue is that --all invokes cargo metadata without --no-deps

let metadata_with_deps = get_cargo_metadata(manifest_path, true)?;

This behavior was introduced in #3664, and the reasoning is spelled out in #3664 (comment). I think this probably requires a fix to upstream, but I'm not sure (ideas welcome!). I figured that regardless, a tracking issue would be useful.

The target issue could conceivably be fixed by passing --filter-platform to cargo metadata (it considers all targets by default). But, that would still only work subject to rust-lang/cargo#8987, and would also mean that cfg-guarded workspace members would not be walked by rustfmt (I think?).

@jonhoo jonhoo added the bug Panic, non-idempotency, invalid code, etc. label Dec 16, 2020
@jonhoo
Copy link
Contributor Author

jonhoo commented Dec 16, 2020

The requisite upstream feature we need is rust-lang/cargo#7483

jonhoo pushed a commit to jonhoo/cargo that referenced this issue Dec 17, 2020
Previously, the location for path-based (i.e., local) packages was
redacted from metadata output. This was done way back when so that
`Cargo.lock` would not include path sources:
rust-lang#7483 (comment)

This PR makes this redaction optional so that `cargo metadata` and
friends _will_ output the source location, which is useful for tools
like `rustfmt` which need to know the paths to local packages (see rust-lang#7483
and rust-lang/rustfmt#4599).

Interestingly enough, no tests fail even though
`disable_path_serialization` is never currently called. I don't know if
that's because there's no test that cover where redaction is needed, or
because redaction is simply no longer needed (presumably because
lockfile generation now uses custom logic). If it is the latter, this PR
can be simplified to not support opting into redaction.

Fixes rust-lang#7483
@calebcartwright
Copy link
Member

Thanks for the report, but I'm going to close as a duplicate as there's a few open issues where this has been discussed already (e.g. #4247) and summarized in #4247 (comment)

@calebcartwright calebcartwright added duplicate and removed bug Panic, non-idempotency, invalid code, etc. labels Dec 17, 2020
@jonhoo
Copy link
Contributor Author

jonhoo commented Dec 17, 2020

I'm terrible at searching, my apologies! I'll try to help pushing on upstream to figure out a fix :)

@calebcartwright
Copy link
Member

No worries, and thanks for your work in this space! It's a tricky issue on both sides and has negative impacts on some portion of the user base so getting this resolved would be fantastic

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

No branches or pull requests

2 participants