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.

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

Accurate deps #498

Merged
merged 37 commits into from
Oct 27, 2023
Merged

Accurate deps #498

merged 37 commits into from
Oct 27, 2023

Conversation

Shnatsel
Copy link
Contributor

@Shnatsel Shnatsel commented Oct 26, 2023 •
edited
Loading

Depends on #496, please review that first!

  • Makes reporting more accurate by excluding dev-dependencies, which cannot affect the production library/binary/etc by design.
    • A flag for including dev-dependencies of the toplevel package only can be added later, if desired, but transitive dev-dependencies definitely should always be omitted.
  • Only includes the dependencies of the specified package, fixing "All" dependencies include all dependencies across all packages #444

@Shnatsel
Add cargo-metadata as a dependency
11fc031 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
Drop logging configuration for Cargo internals
6a85dba 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
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>
@Shnatsel
Implement querying cargo metadata
1c29f67 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
Drop some more #[deny] directives that would break production builds
85225dc 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
WIP conversion of create_sboms() to cargo-metadata
a358ce4 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
convert create_sboms() and create_bom() to cargo-metadata
f7d240c 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
Convert the rest of the generator functions to cargo-metadata
9163729 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
Comment out toplevel/all dependency filtering for now
efc37f2 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
BEHOLD, IT COMPILES
b3f9ad4 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
Purge the last reference to Cargo internals
dbd3367 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
Remove Cargo from the dependency tree
7b52d4a 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
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>
@Shnatsel
Do not assert any specific error message on invalid Cargo.toml in tes…
7342217 
…ts, since we do not control cargo-metadata

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
Add a comment explaining that the configuration does not work correct…
0bdd508 
…ly at present

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
cargo fmt
e519aef 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
Reimplement filtering of top-level dependencies
1b7372e 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
Also reimplement all_dependencies() with cargo-metadata
edc3583 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
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>
@Shnatsel
Re-enable selection of direct deps only or all deps
d61cc75 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
Drop unused import
0623190 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
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>
@Shnatsel
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>
@Shnatsel
Suppress the warning about resolve being unused for now
416914d 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
Omit the toplevel package from the list of components
6a731f5 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel Shnatsel requested a review from a team as a code owner October 26, 2023 16:08
@Shnatsel
top_level_dependencies(): exclude dev-dependencies
b13338e 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
Split filtering out dev-dependencies into its own function
25eb095 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
Split stripping dev-dependencies from a Node into a helper function
f14932d 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
rename 'member' variable to 'root' for clarity
5c1ae6d 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
Implement BFS to filter out dev-dependencies and dependencies of othe…
5f4ebd5 
…r packages

Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
Make BFS actually run - fix the root element already being present
1f20310 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel
Clarify comment
a592eee 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel Shnatsel mentioned this pull request Oct 26, 2023
Merged
@Shnatsel
drop a fulfilled TODO
f6a8203 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel Shnatsel mentioned this pull request Oct 26, 2023
Merged
lfrancke pushed a commit that referenced this pull request Oct 26, 2023
@Shnatsel
Convert to cargo metadata as a backend (#496)
12a9ba0 
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
Merge branch 'main' into accurate-deps
d707809
@Shnatsel
Fix a typo
e0c720c 
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
lfrancke
lfrancke requested changes Oct 27, 2023
View reviewed changes
Copy link
Contributor

@lfrancke lfrancke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only minor stuff again. I tested it against itself and it worked just fine, tests ran as well.

I'll run it against one of our projects now to double check but don't expect any issues.

cargo-cyclonedx/src/generator.rs Show resolved Hide resolved
cargo-cyclonedx/src/generator.rs Outdated Show resolved Hide resolved
cargo-cyclonedx/src/generator.rs Outdated Show resolved Hide resolved
cargo-cyclonedx/src/generator.rs Outdated Show resolved Hide resolved
Shnatsel and others added 2 commits October 27, 2023 14:01
@Shnatsel @lfrancke
Fix typo in comment
1089b28 
Co-authored-by: Lars Francke <lars.francke@stackable.tech>
Signed-off-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
@Shnatsel @lfrancke
apply style suggestion
14e1026 
I'm sure there is a clippy lint for this

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

Tested it against two of our own projects and it seems to fix #444
Output looks good from my spot checks.

@lfrancke lfrancke merged commit d0760a1 into CycloneDX:main Oct 27, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lfrancke lfrancke lfrancke approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Shnatsel @lfrancke