more explicit build dependency handling

[thillux]

I was wondering, why pkg-config is listed in my SBOM.

SBOMs should IMHO only contain dependencies which are not only used during build or development. It would also be possible to check against DependencyKind::Normal.

[Shnatsel]

I don't think there's a one-size-fits-all solution to this. So I don't think excluding them unconditionally is a good idea.

However, what we should do is clearly indicate that these are build-time dependencies. We can set scope = "excluded" for build dependencies, which would indicate that they are not reachable at runtime. I'd be happy to accept a PR with that.

We could also consider adding a command-line option to exclude build dependencies, if them being explicitly marked as such is not sufficient. But we would need a really strong motivation to add yet another configuration knob.

[thillux]

I don't think there's a one-size-fits-all solution to this. So I don't think excluding them unconditionally is a good idea.

However, what we should do is clearly indicate that these are build-time dependencies. We can set scope = "excluded" for build dependencies, which would indicate that they are not reachable at runtime. I'd be happy to accept a PR with that.

We could also consider adding a command-line option to exclude build dependencies, if them being explicitly marked as such is not sufficient. But we would need a really strong motivation to add yet another configuration knob.

Good suggestions. I'd like to have both and I'm currently working on it.

[thillux]

@Shnatsel I hacked a first draft together to familarize myself with the code base.

Is the basic idea correct, to check if a package was used as a normal dependency somewhere in the dependency graph and set it to DependencyKind::Required and DependencyKind::Excluded otherwise?

Is there a easier/faster path to check this?

[thillux]

My current simple test-case looks like this:

[package]
name = "cargo-meta-tests"
version = "0.1.0"
edition = "2021"

[build-dependencies]
rand = "0.8.5"
rand-esdm = "0.1.4"

[dependencies]
rand = "0.8.5"

[dev-dependencies]
rand = "0.8.5"

[thillux]

After looking through my current SBOM, I realized, that my idea for exclusion is probably too simple. Dependencies, which are only used by build dependencies are not marked as excluded.

Shall we perform some additional pruning/indexing or just live with the simple CLI option to drop build dependencies on demand?

If we can settle on some common idea after my tests, I'll cleanup/rewrite the PR.

[Shnatsel]

Shall we perform some additional pruning/indexing

This. I've already written this logic for cargo auditable, you can simply crib it from here: https://github.com/rust-secure-code/cargo-auditable/blob/38b37332dfa3601f90608c1565fbd6e63bd6b08a/auditable-serde/src/lib.rs#L230-L366

[thillux]

@Shnatsel after cribbing from cargo auditable and adding another graph traversal for indexing dependency kinds, this is ready now from my side.

[Shnatsel]

Could you also add tests, to make sure this functionality doesn't break in the future?

[thillux]

Sure. I'll try tomorrow.

[thillux]

Two simple tests were added. Please have a look again.

[Shnatsel]

Yep, that looks good. Thanks a lot!

Nix flake CI is failing, but none of the current maintainers know or use Nix, and it's been effectively unmaintained for a while. I think we should just go ahead and drop it.

[Shnatsel]

That's a pretty large Cargo.lock for the test crate. It would be pretty difficult to debug when something fails, and presents a rather large supply chain attack surface.

Could you make a minimal Cargo.lock file for the tests? It's likely that cargo auditable has some kind of test fixtures for this already. That should also fix the Nix flake CI.

Everything else looks good. Thanks a lot!

[thillux]

Ok. I'll try tomorrow.

[thillux]

Done. Thanks for the hint to cargo auditable.

[Shnatsel]

Unfortunately I have to revert this because it causes an infinite loop when there is a dependency cycle. A reproducing example can be found in #750

[Shnatsel]

FWIW cargo auditable doesn't enter an infinite loop here.

[thillux]

Ok & sorry for this. I had faulty assumptions. My assumption was, that the metadata I get is already acyclic. I just have to add a cycle check to the graph search. I'll fix this in a new PR. I'll leave the command line argument as is, therefore your other PR will still apply.

[Shnatsel]

No, there may be cycles through dev-dependencies. cargo auditable has correct handling for it, so if you just copy over its traversal algorithm it should work fine.
And please add a test for the cyclic graph too; IIRC cargo auditable has one already, so that could be a good starting point.

Thanks you for sticking with this! It is kind of a hard problem.

I'll leave the command line argument as is, therefore your other PR will still apply.

My other PR is not going to be mergeable. Git will make sure of it. So if you think that name is a good idea, I'd be happy if you just integrated that directly.

[thillux]

@Shnatsel I was able to reproduce the infinite loop and the fix was a one-liner in the end. I have to rebase/cherry-pick now and open a new PR tomorrow.