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

Disabled optional weak dependencies end up in Cargo.lock #10801

Open
Tracked by #9
jonasbb opened this issue Jun 30, 2022 · 29 comments
Open
Tracked by #9

Disabled optional weak dependencies end up in Cargo.lock #10801

jonasbb opened this issue Jun 30, 2022 · 29 comments
Labels
A-features2 Area: issues specifically related to the v2 feature resolver A-lockfile Area: Cargo.lock issues C-bug Category: bug S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@jonasbb
Copy link

jonasbb commented Jun 30, 2022

Problem

Lets say I have two crates, with these contents of Cargo.toml. The Rust code does not matter here.

# foo/Cargo.toml

[dependencies]
serde = {version = "1.0.133", optional = true, default-features = false}
time = {version = "=0.3.10", optional = true, default-features = false}
chrono = {version = "=0.4.18", optional = true, default-features = false}

[features]
serialization = ["dep:serde", "time?/serde-well-known"]
# bar/Cargo.toml

[dependencies]
foo.features = ["serialization"]
foo.path = "../foo"

# time = "=0.3.11"
# chrono = "=0.4.19"

As you can see, bar depends on foo and enables the serialization feature, but it does not enable the time nor the chrono feature.

This results in the following bar/Cargo.lock file:

bar/Cargo.lock
# This file is automatically @generated by Cargo.
# It is not intended for manual editing.
version = 3

[[package]]
name = "app"
version = "0.1.0"
dependencies = [
 "foo",
]

[[package]]
name = "foo"
version = "0.1.0"
dependencies = [
 "serde",
 "time",
]

[[package]]
name = "itoa"
version = "1.0.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "112c678d4050afce233f4f2852bb2eb519230b3cf12f33585275537d7e41578d"

[[package]]
name = "libc"
version = "0.2.126"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "349d5a591cd28b49e1d1037471617a32ddcda5731b99419008085f72d5a53836"

[[package]]
name = "num_threads"
version = "0.1.6"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "2819ce041d2ee131036f4fc9d6ae7ae125a3a40e97ba64d04fe799ad9dabbb44"
dependencies = [
 "libc",
]

[[package]]
name = "serde"
version = "1.0.137"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "61ea8d54c77f8315140a05f4c7237403bf38b72704d031543aa1d16abbf517d1"

[[package]]
name = "time"
version = "0.3.10"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "82501a4c1c0330d640a6e176a3d6a204f5ec5237aca029029d21864a902e27b0"
dependencies = [
 "itoa",
 "libc",
 "num_threads",
 "serde",
]

The relevant snippet is at the end:

[[package]]
name = "time"
version = "0.3.10"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "82501a4c1c0330d640a6e176a3d6a204f5ec5237aca029029d21864a902e27b0"
dependencies = [
 "itoa",
 "libc",
 "num_threads",
 "serde",
]

Here the dependency time is declared, even though it is an optional dependency and nothing enabled the corresponding feature on foo. The time dependency even has serde listed as one of its dependencies too.


I think this is problematic in a couple of ways.

It bloats the Cargo.lock and makes it imply that changes are larger than they actually are. In the above example adding foo will also add time even if that is not necessary.

Dependency management tools will get confused by this. I mean tools like dependabot or cargo audit, which use the Cargo.lock as source of truth. Since these tools will see an entry for time, even if it is unused, they will create pull requests or warn about security vulnerabilities which do not exists.

This prevents using two crates with conflicting version requirements. You can see what happens if you uncomment the lines in bar/Cargo.toml.
Uncommenting time will fail, since now two conflicting version constraints (=0.3.10 != =0.3.11) exist.
However, uncommenting chrono works with the conflicting versions (=0.4.18 != =0.4.19).

Steps

  1. Create project foo with the above content for Cargo.toml.
  2. Create project bar with the above content for Cargo.toml.
  3. Observe how the Cargo.lock of bar contains an entry for time and even a transitive serde dependency in time.

Possible Solution(s)

Disabled optional dependencies should not be tracked in Cargo.lock, even if there is a weak dependency linking to it. This brings it in line with "normal" optional dependencies.

Notes

While the time entry exists in the Cargo.lock, it appears cargo correctly ignores it when building the code. There is neither a Compiling time ... log line nor are there any artifacts in the target folder associated with time.

I checked various issues and documentation entries, but I couldn't find if this is a bug or intended.

The tracking issue (#8832) and documentation (https://doc.rust-lang.org/nightly/cargo/reference/features.html#dependency-features) do not mention any changes in to Cargo.lock. Neither does the implementation PR (#8818), which does not even seem to have any lock files.

Version

cargo 1.62.0 (a748cf5a3 2022-06-08)
release: 1.62.0
commit-hash: a748cf5a3e666bc2dcdf54f37adef8ef22196452
commit-date: 2022-06-08
host: x86_64-unknown-linux-gnu
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.80.0-DEV (sys:0.4.51+curl-7.80.0 vendored ssl:OpenSSL/1.1.1n)
os: OracleLinux 36.0.0 [64-bit]


FYI: My OS is Fedora 36, but this is the output of `cargo version --verbose`.
@jonasbb jonasbb added the C-bug Category: bug label Jun 30, 2022
@ehuss ehuss added A-lockfile Area: Cargo.lock issues A-features2 Area: issues specifically related to the v2 feature resolver labels Jun 30, 2022
@Jake-Shadle
Copy link
Contributor

Note that this bug also affects the output of the metadata subcommand, but not the tree subcommand. Weak dependencies are resolved in the resolve.nodes map even if they haven't been enabled by the crate feature.

N3xed added a commit to esp-rs/esp-idf-sys that referenced this issue Jul 31, 2022
cargo metadata resolves all dependencies even ones that are disabled (known as weak dependencies)
which cargo build doesn't do (see issue rust-lang/cargo#10801). These unresolved dependencies require
network access to load their package metadata.
As the `--frozen` flag itself blocks network access (in addition to blocking changes to the Cargo.lock)
too, we need to remove both the `--frozen` and `--offline` flags. Doing this allows cargo the change the
`Cargo.lock` though, which is undesirable (as we're in the process of building the same dependencies that
rely on the `Cargo.lock` in the first place).
@trevor-crypto
Copy link

I believe I am running into this with target-specific dependencies:

[target.'cfg(target_arch = "wasm32")'.dependencies]
wasm-bindgen = "=0.2.78"

I noticed this when there was a version conflict (since I already had the dependency in the lock file from another dependency).

JakobLachermeier pushed a commit to JakobLachermeier/esp-idf-sys that referenced this issue Aug 26, 2022
cargo metadata resolves all dependencies even ones that are disabled (known as weak dependencies)
which cargo build doesn't do (see issue rust-lang/cargo#10801). These unresolved dependencies require
network access to load their package metadata.
As the `--frozen` flag itself blocks network access (in addition to blocking changes to the Cargo.lock)
too, we need to remove both the `--frozen` and `--offline` flags. Doing this allows cargo the change the
`Cargo.lock` though, which is undesirable (as we're in the process of building the same dependencies that
rely on the `Cargo.lock` in the first place).
@lopopolo
Copy link

lopopolo commented Sep 8, 2022

I'm not sure but I think I ran into this issue in:

I found it unexpected that serde appears in the lockfile since I am specifically enabling and disabling features such that I avoid pulling in serde. I was extra confused that serde does not appear in cargo tree.

@ljedrz
Copy link

ljedrz commented Oct 13, 2022

I've noticed this as well when cargo update caused the lockfile to balloon more than I expected; it was caused by one of the indirect dependencies having introduced a [target.'cfg(target_os = "haiku")'.dependencies] section.

@lopopolo
Copy link

lopopolo commented Oct 13, 2022

@ljedrz that behavior is expected. If you're talking about iana-time-zone that dependency is not optional. It is a required dependency that is only used on a specific platform. Cargo adds all required dependencies to the lockfile even if they are cfg'd.

It's the same reason winapi appears in the lockfile even if you are building on Linux.

@ljedrz
Copy link

ljedrz commented Oct 13, 2022

@lopopolo indeed, it's that dependency; however, since I'm not building on an applicable target, I'd expect it to not show up in my lockfile just as much as if it were an optional (and disabled) dependency.

jonasbb added a commit to jonasbb/c-dns that referenced this issue Oct 30, 2022
Most of the new dependencies are false positive and are not actually
required, but are false dependencies due to a Cargo bug.
rust-lang/cargo#10801
rpjohnst added a commit to rpjohnst/dejavu that referenced this issue Nov 12, 2022
A note: this adds serde to the lockfile, but it is not actually used. This
is caused by rust-lang/cargo#10801.
@Sytten
Copy link

Sytten commented Nov 18, 2022

This also happens with sea-orm which pulls chrono and time when you use the with-json flag.
https://github.com/SeaQL/sea-orm/blob/master/Cargo.toml#L75
This is really wrong.

@memoryruins
Copy link

memoryruins commented Nov 18, 2022

@jonasbb

I checked various issues and documentation entries, but I couldn't find if this is a bug or intended.

I believe this is a known limitation of the lock file generation that is intended to be improved eventually. From #5655 (comment),

lockfile generation requires the assumption that all possible features are enabled. Generating a lockfile without optional features is covered by #5133

While #5133 mentions --minimal-cargo-lock, it doesn't appear to be implemented yet. This should also help with offline usage #10352 and platform specific vendoring #7058 (similarly, target specific is brought up in #11313)

From #7058 (comment),

Cargo currently requires that the lock file is platform-independent, meaning that the lock file (and generating the lock file) always has to locate all crates for all possible platforms. [...] There's a tracking issue for this at #5133 which is a possible way to pass a flag to Cargo and say "please generate a minimal lock file", and with that we can safely delete all the dependencies for other platforms.

@jonasbb
Copy link
Author

jonasbb commented Nov 18, 2022

@memoryruins

I checked various issues and documentation entries, but I couldn't find if this is a bug or intended.

I believe this is a known limitation of the lock file generation that is intended to be improved eventually. From #5655 (comment),

This only applies to the root crate (the one you are working on) but not the dependencies. When you depend on serde without extra features, then the optional serde_derive and any proc-macro dependencies are not included, since the feature is not enabled.

Cargo.toml
[package]
name = "foobar"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
serde = "1.0.147"
Cargo.lock
# This file is automatically @generated by Cargo.
# It is not intended for manual editing.
version = 3

[[package]]
name = "foobar"
version = "0.1.0"
dependencies = [
 "serde",
]

[[package]]
name = "serde"
version = "1.0.147"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "d193d69bae983fc11a79df82342761dfbf28a99fc8d203dca4c3c1b590948965"

From #7058 (comment),

Cargo currently requires that the lock file is platform-independent, meaning that the lock file (and generating the lock file) always has to locate all crates for all possible platforms. [...] There's a tracking issue for this at #5133 which is a possible way to pass a flag to Cargo and say "please generate a minimal lock file", and with that we can safely delete all the dependencies for other platforms.

This issue is not comparable to platform dependent Cargo.lock. The issue presented here covers dependencies which are never used, on no platform and also not if using --all-features.

In my original posts in the bar/Cargo.lock snippet, you find chrono to be missing, while time is included. Here you see again that optional dependencies are already excluded from Cargo.lock. The only difference between these two is the existence of the weak dependency time?/serde-well-known. So I would put the blame here on the implementation of weak features. But as explained in the original post, I could find no comments on if these Cargo.lock changes are intentional or not.

@memoryruins
Copy link

@jonasbb

I understand what you mean with weak features now. I'll see if I can find anything mentioned in the past.

This issue is not comparable to platform dependent Cargo.lock.

Apologies for any confusion, I forgot to add an @ mention to the platform specific part of my comment, which was meant to be in response to @ljedrz #10801 (comment)

since I'm not building on an applicable target, I'd expect it to not show up in my lockfile just as much as if it were an optional (and disabled) dependency.

@Sytten
Copy link

Sytten commented Nov 18, 2022

The issue with platforms should be be a separate issue, this is really for the issue where a dependency is truly never used but still included. Let's keep it on topic 👍

@memoryruins
Copy link

@Sytten

The issue with platforms should be be a separate issue

Indeed, I linked to those earlier in #10801 (comment) so that related discussion can move there.

@epage epage changed the title Optional and disabled dependencies end up in Cargo.lock Optional and disabled weak dependencies end up in Cargo.lock Nov 26, 2022
@epage epage changed the title Optional and disabled weak dependencies end up in Cargo.lock Disabled Optional weak dependencies end up in Cargo.lock Nov 26, 2022
@epage epage changed the title Disabled Optional weak dependencies end up in Cargo.lock Disabled optional weak dependencies end up in Cargo.lock Nov 26, 2022
@lilith
Copy link
Contributor

lilith commented Aug 22, 2024

Soo... ring isn't in my 'cargo tree', but it is still trying and failing to compile. I think this bug has worsened.

@weihanglo
Copy link
Member

Soo... ring isn't in my 'cargo tree', but it is still trying and failing to compile. I think this bug has worsened.

@lilith Optional dependencies shouldn't compile if not activated. It might be something enabling ring and cargo-tree has a bug not reporting it. Please open a separate issue with reproduction and we can investigate then.

@ErichDonGubler
Copy link
Contributor

The Firefox WebGPU team is running into this issue transitively because of once_cell's optional dependency on the sizable portable-atomic crate (see matklad/once_cell#265 (comment). This is particularly painful for us because of a strict cargo vet policy for dependencies in the Mozilla monorepo.

CC @jimb.

@epage
Copy link
Contributor

epage commented Oct 4, 2024

Would that be a bug to fix in cargo vet?

@jimblandy
Copy link

jimblandy commented Oct 4, 2024

@epage We've presented this to the cargo vet maintainers, and they said:

This isn't us, the dependency edge appears in cargo metadata

{
  "id": "registry+https://github.com/rust-lang/crates.io-index#[email protected]",
  "dependencies": [
    "registry+https://github.com/rust-lang/crates.io-index#[email protected]"
  ],
  "deps": [
    {
      "name": "portable_atomic",
      "pkg": "registry+https://github.com/rust-lang/crates.io-index#[email protected]",
      "dep_kinds": [
        {
          "kind": null,
          "target": null
        }
      ]
    }
  ],
  "features": [
    "alloc",
    "default",
    "race",
    "std"
  ]
},

There is perhaps some dependency feature unification across multiple targets which is causing this feature to be enabled
There's nothing we can do about that on the cargo-vet side because of the data we get from cargo metadata

@jimblandy
Copy link

jimblandy commented Oct 4, 2024

There are a lot of tools that depend on cargo metadata getting this right. In addition to cargo vet, as previously mentioned, this also causes cargo vendor to incorporate the unused crate into the tree.

@est31
Copy link
Member

est31 commented Oct 5, 2024

As explained in my (failed) PR #11938, cargo has two resolvers, and only one of the two has this problem. The other resolver doesn't. I wonder if one could add a mode to cargo metadata to use the "dependency resolver" instead of the "feature resolver".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-features2 Area: issues specifically related to the v2 feature resolver A-lockfile Area: Cargo.lock issues C-bug Category: bug S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
Development

Successfully merging a pull request may close this issue.