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

Optional dependencies #537

Open
cemoktra opened this issue Aug 28, 2023 · 2 comments
Open

Optional dependencies #537

cemoktra opened this issue Aug 28, 2023 · 2 comments

Comments

@cemoktra
Copy link

Hi,

for a project i checked the workload i would have to do an audit and noticed that i should audit a crate (500k lines to audit) that comes in due to an optional/feature dependency. As the feature is not enabled, this crate is obviously not used.

It would be good if such crates would not appear in the list of suggested audits

@TitanNano
Copy link

TitanNano commented Aug 30, 2023

Digging into cargo metatdata I figured out what is the issue here. Cargo does not care about optional sub dependencies and always returns all of them when generating the metadata.

With this minimal cargo file:

[package]
name = "dependency-test"
version = "0.1.0"
edition = "2021"

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

[dependencies]
sqlx = { version = "0.7.1", features = ["postgres"], optional = true }

[features]
db = ["dep:sqlx"]

It's easily reproducible that the db feature enables all optional sub dependencies:

$ cargo metadata --features db | jq '.resolve.nodes | map(select(.deps[].name == "sqlx_mysql" or .deps[].name == "sqlx")) | map({ id: .id, features: .features, deps: .deps | map(select(.name == "sqlx_mysql")) })'
[
  {
    "id": "dependency-test 0.1.0 (path+file:///home/jovan/dependency-test)",
    "features": [
      "db"
    ],
    "deps": []
  },
  {
    "id": "sqlx 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)",
    "features": [
      "any",
      "default",
      "json",
      "macros",
      "migrate",
      "postgres",
      "sqlx-macros",
      "sqlx-postgres"
    ],
    "deps": [
      {
        "name": "sqlx_mysql",
        "pkg": "sqlx-mysql 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)",
        "dep_kinds": [
          {
            "kind": null,
            "target": null
          }
        ]
      }
    ]
  },
  {
    "id": "sqlx-macros-core 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)",
    "features": [
      "default",
      "json",
      "migrate",
      "postgres",
      "sqlx-postgres"
    ],
    "deps": [
      {
        "name": "sqlx_mysql",
        "pkg": "sqlx-mysql 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)",
        "dep_kinds": [
          {
            "kind": null,
            "target": null
          }
        ]
      }
    ]
  }
]

while removing the feature gets rid of anything sqlx related:

$ cargo metadata | jq '.resolve.nodes | map(select(.deps[].name == "sqlx_mysql" or .deps[].name == "sqlx")) | map({ id: .id, features: .features, deps: .deps | map(select(.name == "sqlx_mysql")) })'
[]

Unfortunately cargo metadata does not indicate at all which dependencies are being enabled by a feature. Looking at the data in .packages though it should be possible to only consider optional dependencies for a package, if the package has an enabled feature which contains dep:some-package.

// .resolve.nodes
{
  "id": "dependency-test 0.1.0 (path+file:///home/jovan/dependency-test)",
  "features": [
    "db",   // <-- enabled feature
    "sqlx"  // <-- enabled feature
  ],
  "deps": []
},

// .packages
{
    "name": "dependency-test",
    "version": "0.1.0",
    "id": "dependency-test 0.1.0 (path+file:///home/jovan/dependency-test)",
    "license": null,
    "license_file": null,
    "description": null,
    "source": null,
    "dependencies": [
// ... not relevant
    ],
    "targets": [
// ... not relevant
    ],
    "features": {
      "db": [      // <-- is currently enabled
        "sqlx"     // <-- does not have a dep: sub-feature, so not relevant
      ],
      "sqlx": [    // <-- is currently enabled
        "dep:sqlx" // <-- has a dep: sub-feature, this should mark the sqlx dependency as actually active.
      ]
    },
    "manifest_path": "/home/jovan/dependency-test/Cargo.toml",
    "metadata": null,
    "publish": null,
    "authors": [],
    "categories": [],
    "keywords": [],
    "readme": null,
    "repository": null,
    "homepage": null,
    "documentation": null,
    "edition": "2021",
    "links": null,
    "default_run": null,
    "rust_version": null
}

TitanNano added a commit to TitanNano/cargo-vet that referenced this issue Sep 5, 2023
`cargo metadata` currently outputs all dependencies, even optional
dependencies that are not active. By comparing `dep:*` features with
dependencies that have been declared as optional, we can eliminate
inactive optional dependencies.
@mystor
Copy link
Collaborator

mystor commented Sep 14, 2023

This appears to be caused by an issue with how cargo resolves dependencies in the presence of optional features being conditionally enabled in optional dependencies. If you have a series of crates like:

[package]
name = "a"

[dependencies]
b = { ..., features = ["feat"] }
[package]
name = "b"

[dependencies]
c = { ..., optional = true }

[features]
feat = ["c?/feature"]
[package]
name = "c"

[features]
feature = []

Then the crate c can never be a dependency of a, however it will be included in a's Cargo.lock, and will be listed as an enabled dependency edge within cargo metadata output. I think this might be a bug in cargo, as if you remove the c?/feature, then this doesn't happen.

I'd like to avoid cargo vet getting into the business of interpreting cargo's features or dependency chains, as that's a lot of complexity which would otherwise be unnecessary to add.

The best place to fix this would probably be in Cargo itself. I'm guessing that the crate is appearing as a dependency because the dependency needs to be resolved in order to validate that the named feature is present, and resolving the dependency has the side effect of adding it to the graph in this way, even if it is not used (similar to how dependencies for other platforms are added).

It appears that fixing this within cargo-vet would require us to manually re-resolve dependency and feature requirements from the crate's metadata definition, then match those resolved dependencies against the list of resolved dependencies produced by cargo metadata to find dependencies which should be discarded due to being unreachable.

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

No branches or pull requests

3 participants