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.

Sign up for GitHub

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

Jump to bottom

Convert to `cargo metadata` as a backend #496

Merged

lfrancke merged 26 commits into CycloneDX:main from Shnatsel:cargo-metadata

Oct 26, 2023

Contributor

Shnatsel commented Oct 26, 2023 •

edited

Loading

This PR switches to the API-stable cargo metadata as opposed to the previous direct dependency on unstable internal APIs of Cargo.

It is mostly bug-compatible, with the actual fixes to come in subsequent PRs.

Benefits

Far smaller binary size
Shorter compile times
Reliance only on stable APIs will require far less maintenance going forward
Download crates in parallel #489 (we got it for free by switching to cargo metadata)

Fixes

One of the tests was creating an invalid dependency that was getting ignored by Cargo, but still included in the SBOM. This is now fixed, and I've updated the test to create a proper dependency.

Behavior changes

The previous code was inconsistent on what kinds of dependencies it includes. It previously included normal only (not build!) in top-level only mode, and all kinds (even dev!) in all deps mode. Now all kinds of dependencies are included always - it's still wrong, but at least it's consistent. Proper filtering by mode and package is implemented in a subsequent PR: #498

Regressions

Reading configuration from Cargo.toml is not fully wired up, although its tests pass. I've skipped it for now and left a comment because this behavior seems to be a bad idea in the first place: #495

Shnatsel requested a review from a team as a code owner

October 26, 2023 06:16

Shnatsel added 25 commits

October 26, 2023 07:17


          Add cargo-metadata as a dependency

11fc031

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Drop logging configuration for Cargo internals

6a85dba

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Drop #[deny] directives for warnings and lints. These should not be u…

79a5c52

…sed in production - they will cause the build to fail on a newer compiler if new warnings or lints are added. The right way to do this is configure CI.

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Implement querying cargo metadata

1c29f67

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Drop some more #[deny] directives that would break production builds

85225dc

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          WIP conversion of create_sboms() to cargo-metadata

a358ce4

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          convert create_sboms() and create_bom() to cargo-metadata

f7d240c

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Convert the rest of the generator functions to cargo-metadata

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Comment out toplevel/all dependency filtering for now

efc37f2

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          BEHOLD, IT COMPILES

b3f9ad4

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Purge the last reference to Cargo internals

dbd3367

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Remove Cargo from the dependency tree

7b52d4a

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Re-enable reading config from Cargo.toml, even though this seems like…

c398423

… a bad idea

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Do not assert any specific error message on invalid Cargo.toml in tes…

…ts, since we do not control cargo-metadata

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Add a comment explaining that the configuration does not work correct…

0bdd508

…ly at present

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          cargo fmt

e519aef

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Reimplement filtering of top-level dependencies

1b7372e

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Also reimplement all_dependencies() with cargo-metadata

edc3583

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Make top_level_dependencies() consistent with all_dependencies() in o…

e0a4cb5

…utputting the toplevel package in resolve

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Re-enable selection of direct deps only or all deps

d61cc75

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Drop unused import

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Include the root package in the output of top_level_dependencies() an…

ade4077

…d all_dependencies(), it is required for correctness

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Fix a test that was creating an invalid dependency without a lib targ…

39b3426

…et, which was ignored by cargo

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Suppress the warning about resolve being unused for now

416914d

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>


          Omit the toplevel package from the list of components

6a731f5

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>

Shnatsel force-pushed the cargo-metadata branch from cd96562 to 6a731f5 Compare

October 26, 2023 06:17

This was referenced Oct 26, 2023

Accurate deps #498

Merged

Fix spec compliance: use an actually unique bom-ref #503

Merged

Add prod build and release steps #488

Merged

Shnatsel mentioned this pull request

Write the dependency tree to the SBOM #504

Merged

Contributor

lfrancke commented Oct 26, 2023

Thank you for the PR!

I did take a quick look and have one initial question: We currently run cargo metadata once for everything. As far as I know it can be run against each package individually.
Wouldn't that solve the issues we currently have?
Yes, it'd mean running cargo metadata multiple times so it'd be a bit slower but that's a tradeoff I'd be happy to take.

Disclaimer: I have not looked at your follow-up PRs yet.

Contributor Author

Shnatsel commented Oct 26, 2023

Even if we ran it for every package separately, we would still have to do the graph traversal to filter out the transitive dev-dependencies. That's implemented in #498. So running cargo metadata per-package doesn't really get us anything.

lfrancke requested changes

View reviewed changes

Contributor

lfrancke left a comment

Minor stuff
It's really really hard to not comment on some of the obvious wrong things :)

cargo-cyclonedx/src/generator.rs Show resolved Hide resolved

cargo-cyclonedx/src/generator.rs Outdated Show resolved Hide resolved

cargo-cyclonedx/src/generator.rs Show resolved Hide resolved


          Fix typo in comment

819b269

Co-authored-by: Lars Francke <lars.francke@stackable.tech>
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>

Shnatsel force-pushed the cargo-metadata branch from d3dedaf to 819b269 Compare

October 26, 2023 21:47

Shnatsel requested a review from lfrancke

October 26, 2023 21:48

lfrancke approved these changes

View reviewed changes

lfrancke merged commit 12a9ba0 into CycloneDX:main

9 checks passed

This was referenced Oct 27, 2023

Download crates in parallel #489

Closed

[Feature Request]: Add support for building BOMs from cargo build's --build-plan output #175

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet