Use cargo_metadata to explore the dependency tree.

[anderejd]

Switch out the cargo API pieces currently used for exploring the dependency graph and use https://crates.io/crates/cargo_metadata instead.

Found here: sfackler/cargo-tree#41

This is a subtask of #69

Consider using https://crates.io/crates/krates as part of this migration.

[jmcconnell26]

Hi,

I was looking to start working on this issue, and raised this PR
#110
off the back of this comment
https://github.com/rust-secure-code/cargo-geiger/blob/master/cargo-geiger/src/cli.rs#L138

Would you have any objections to me working on this?

Thanks
Josh

[tarcieri]

You might also consider cargo-lock, which also has an integrated petgraph-based dependency graph.

[anderejd]

Hello,

Any and all contributions are very much welcome!

In fact, I have been planning to reach out for new contributors for some time, but somehow continuously have been finding excuses to do it later. For some background see #87

The PR looks nice and clean, will merge in a moment.

[anderejd]

@tarcieri What's the main differences between cargo-lock and cargo_metadata?

[jmcconnell26]

Hey,

That's fantastic, cheers! I'd definitely be keen to start landing a few PR's where there's help needed.

[anderejd]

If we can eliminate cargo as a compile time dependency that would be huge. Keep those PRs coming!

[tarcieri]

@anderejd cargo-lock parses and consumes Cargo.lock directly as a self-contained library. In that regard it should be marginally faster than cargo_metadata. If you intend to use any other output from cargo_metadata than the crate dependency graph though, it might be better.

cargo_metadata might also work a bit more automatically than cargo-lock in a workspace context.

[Shnatsel]

Sadly cargo-lock doesn't cut it in this particular case because it does not allow distinguishing between runtime, build and dev dependencies. And we don't want to search build and development dependencies for unsafe code. Build dependencies have a completely different threat model: unsafe code in build dependencies is not dangerous, but e.g. network access or filesystem operations could be.

Resolving dependency types is surprisingly tricky - cargo_metadata shows the raw data from the dependency graph, so that a runtime dependency of a crate that your crate has as a build dependency will still be considered a runtime dependency, while we want to consider it a build dependency for geiger purposes. I have recently implemented splitting crates into build, dev and runtime dependencies relative to the toplevel crate here; feel free to steal it.

If you need any more info on this, feel free to ping me on Zulip.

[jmcconnell26]

I started off with an attempt at refactoring the cli module into its constituent parts, with a view to adding unit tests to ensure that I don't break any functionality.
#111
I hope this looks OK, please let me know if there are any changes you would like me to make.
Thanks,
Josh

[jmcconnell26]

After the refactoring from the previous PR, I've added in a few unit tests here:
#112
I hope you think this looks OK, it's been very useful for me to get to grips with the code base.
If there are any changes you would like me to make, please let me know.
Thanks,
Josh

[anderejd]

#112 looks great! Merged.

[jmcconnell26]

Once the change from #139 has been made, the majority of the usages of cargo::core should be contained to the utils module (which I'm not thrilled on the name of, and would like to rename to mapping if there are no objections?).
This will leave the following tasks to be completed before the cargo::core dependency can be removed:

• Get the ManifestMetadata from a cargo_metadata::Package or cargo_metadata::PackageId
• Create a function that returns the same result as deps_not_replaced on PackageSet without using cargo::core
• Handle extracting the root package from a virtual manifest using cargo_metadata
• Create a function that returns the same result as replace on PackageId
• Create a function to construct cargo_geiger_serde::PackageId from cargo_metadata::PackageId

[anderejd]

the utils module (which I'm not thrilled on the name of, and would like to rename to mapping if there are no objections?)

@jmcconnell26 mapping is certainly a better name, let's rename it.

[jmcconnell26]

Awesome! Have landed #140 to make the change.

[anderejd]

@jmcconnell26 Do you know of anything more that needs to be done before we close this issue?

[jmcconnell26]

@anderejd, I think this should be everything on this issue.
I have left some usages of the cargo::core packages in the dev dependencies only of the mapping module, to be used to ensure that there is feature parity when unit testing.
I can pull together a list of the remaining cargo crate usages for the parent issue if that would be helpful?

[anderejd]

I have left some usages of the cargo::core packages in the dev dependencies only of the mapping module, to be used to ensure that there is feature parity when unit testing.
I can pull together a list of the remaining cargo crate usages for the parent issue if that would be helpful?

@jmcconnell26 A comment informing about the purpose of testing for each such cargo::core usage could make it easier to remove it later when implementing #69.