rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.56k stars 2.38k forks source link

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

Open jonasbb opened 2 years ago

jonasbb commented 2 years ago

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 ```toml # 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 (https://github.com/rust-lang/cargo/issues/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 (https://github.com/rust-lang/cargo/pull/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`.
Jake-Shadle commented 2 years ago

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.

trevor-crypto commented 2 years ago

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).

lopopolo commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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.

Sytten commented 1 year ago

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 commented 1 year ago

@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 https://github.com/rust-lang/cargo/issues/5655#issuecomment-408633070,

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 https://github.com/rust-lang/cargo/issues/7058#issuecomment-539845446,

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 commented 1 year ago

@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 ```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 ```toml # 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 commented 1 year ago

@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 https://github.com/rust-lang/cargo/issues/10801#issuecomment-1277842065

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 commented 1 year ago

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 commented 1 year ago

@Sytten

The issue with platforms should be be a separate issue

Indeed, I linked to those earlier in https://github.com/rust-lang/cargo/issues/10801#issuecomment-1320376112 so that related discussion can move there.

glandium commented 1 year ago

Another side-effect of this problem is that cargo vendor vendors extra unnecessary dependencies.

jonasbb commented 1 year ago

Users are running into situations where cargo can no longer resolve their dependencies due to this bug. For example, this issue https://github.com/jonasbb/serde_with/issues/550 where a pinned chrono version together with this bug results in error: failed to select a version for chrono..

loganmzz commented 1 year ago

Not sure if issue is the same but here is my case:

I started a project using actix-web 3.3.3 which has strong dependency to actix-http ^2.2.0. And actix-http 2.2.2 has strong dependency to base64 ^0.13 (resolved to 0.13).

Bump to actix-web 4.3.1, I now have strong dependency to actix-http 3.3.0 which has optional dependecy to base64 ^0.21.

However, as I don't depend explicitly to base64 I would have expected this transitive dependency to be removed. As base64 0.21 require 2021 edition instead of 2018 (use by my project and Actix components). I can't build my project due to:

error: failed to download `base64 v0.21.0`

Caused by:
  unable to get packages from source

Caused by:
  failed to parse manifest at `/home/loganmzz/.cargo/registry/src/github.com-1ecc6299db9ec823/base64-0.21.0/Cargo.toml`

Caused by:
  feature `edition2021` is required

  this Cargo does not support nightly features, but if you
  switch to nightly channel you can add
  `cargo-features = ["edition2021"]` to enable this feature

Any solution to clean the Cargo.lock ?

est31 commented 1 year ago

@ehuss you wrote here:

For now, the resolver must assume that the dependency is used, since that information isn't available until after dependency resolution.

I'm not sure I agree, as I've thought that the information is available in the index. I've just confirmed that the json contains the ?. So the resolver, which already knows which features are enabled and which aren't, needs to just be made to respect the ? instead of treating it as if the ? weren't present (and instead treating foo?/bar as foo/bar).

IMO this can easily be addressed by a PR with a new Cargo.lock ResolveVersion (like done in 7dd9872c13e31b5782ec37dee05f35abab6b6f92 ), or alternatively, using a resolver = 3 like mechanism (I'd prefer the former as there seems to be consensus that these deps don't belong into Cargo.lock).

Edit: so it seems that the new resolver is able to work with weak dependency features, but the old resolver (which seems to be the one that generates the Cargo.lock, even if the new resolver is active) is not working with weak dependency features. I think letting the new resolver unconditionally determine Cargo.lock is not the best idea, due to its per-target behaviour, but maybe one could change the old resolver to respect weak dependencies.

est31 commented 1 year ago

Filed a PR: #11938

Sytten commented 1 year ago

Is there a champion for this fix in the cargo WG? It is a bug that has been lingering for more than a year now and the current proposed resolution is stuck as per https://github.com/rust-lang/cargo/pull/11938#issuecomment-1623769819. It's clear the community has expressed this is a concern and a blocker for adoption of the weak dependencies.

Nemo157 commented 9 months ago

Right now this is causing a bit of an advisory spam, https://rustsec.org/advisories/RUSTSEC-2023-0071.html impacts the rsa crate, which is used by sqlx-mysql, but this is getting detected on every crate that uses sqlx even without mysql because of the weak-dependency-features pulling it into the lockfile.

matthewgapp commented 8 months ago

This is blocking for us as I know no workaround that doesn't involve forking one or more crates that conflict.

With this example repo: https://github.com/matthewgapp/rust-lang-issue-10801), you'll be able to reproduce the below error. Note that while the sqlsync crate does depend on sqlite the sqlx crate shouldn't depend on sqlite (note the weak references). But cargo thinks it does.

error: failed to select a version for `libsqlite3-sys`.
    ... required by package `sqlx-sqlite v0.7.3`
    ... which satisfies dependency `sqlx-sqlite = "=0.7.3"` (locked to 0.7.3) of package `sqlx v0.7.3`
    ... which satisfies dependency `sqlx = "^0.7.3"` (locked to 0.7.3) of package `rust-lang-issue-10801 v0.1.0 (/Users/matthewgapp/code/scratchpad/rust-lang-issue-10801)`
versions that meet the requirements `^0.27.0` (locked to 0.27.0) are: 0.27.0

the package `libsqlite3-sys` links to the native library `sqlite3`, but it conflicts with a previous package which links to `sqlite3` as well:
package `libsqlite3-sys v0.26.0 (https://github.com/trevyn/rusqlite?branch=wasm32-unknown-unknown#463090d3)`
    ... which satisfies git dependency `libsqlite3-sys` of package `sqlsync v0.2.0 (https://github.com/matthewgapp/sqlsync#c34cb11f)`
    ... which satisfies git dependency `sqlsync` of package `rust-lang-issue-10801 v0.1.0 (/Users/matthewgapp/code/scratchpad/rust-lang-issue-10801)`
Only one package in the dependency graph may specify the same links value. This helps ensure that only one copy of a native library is linked in the final binary. Try to adjust your dependencies so that only one package uses the links ='libsqlite3-sys' value. For more information, see https://doc.rust-lang.org/cargo/reference/resolver.html#links.

failed to select a version for `libsqlite3-sys` which could resolve this conflict

The offending cargo toml has these dependencies:

[dependencies]
sqlx = { version = "0.7.3", features = [ "runtime-tokio-rustls", "postgres", "chrono"], default-features = false }
sqlsync =  { git = "https://github.com/matthewgapp/sqlsync" }
beurdouche commented 6 months ago

I just had this problem when trying to vendor code in mozilla-central. One of the crate has massive dependencies which were pulled in for vetting and vendoring while not being used. I had to fork the crate to manually remove the unused dependencies and use the fork but that's not really sustainable.

qsantos commented 5 months ago

Coming from https://github.com/launchbadge/sqlx/issues/2911 from the exact same thing: cargo audit mistakenly reports a vulnerability in the rsa package even though it is not in cargo tree.

lilith commented 3 weeks ago

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

weihanglo commented 3 weeks ago

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.