component-detection icon indicating copy to clipboard operation
component-detection copied to clipboard

Change Rust/Cargo detector to be lock file based

Open danielframpton opened this issue 3 years ago • 10 comments

This is a significant change to Rust component detection to solely use the Cargo.lock file to detect component usage (e.g., no longer processing Cargo.toml).

As discussed in #116 there are several issues with the existing Cargo.toml processing that this change sidesteps by reporting all crates from Cargo.lock.

This change also explicitly includes the package registry in the identifier for components. I have not investigated what implications there may be for this to downstream tools, but because Cargo has support for multiple registries the package name and version number is insufficient to identify the package (because of multiple registries or a local package with the same name as one published in the registry).

In general, the sole use of the Cargo.lock file should strictly increase the number of crates reported, although I did observe some reductions that were due to this change no longer reporting some local/path crates as components (because they had a version and path specified) but I believe this was unintentional/unexpected.

There was also significant duplication in the code between v1 and v2 detectors, but in rewriting the logic to process the lock file I was able to use the same logic for both formats.

I expect this to need a careful review and some changes before landing, but I wanted to send out the PR to make some progress on getting these issues resolved.

Fixes #116

danielframpton avatar May 08 '22 23:05 danielframpton

Hey @arlosi and @danielframpton, is there anything else blocking this pr? It would be nice to have component detection in good shape for rust. Thanks!

juvazq avatar Jul 29 '22 21:07 juvazq

It needs review from the maintainers. Neither @danielframpton nor I can approve it.

arlosi avatar Jul 29 '22 21:07 arlosi

Thanks @arlosi,

@jcfiorenzano @cobya do you think this can be merged? Do you miss something before it can be approved?

juvazq avatar Aug 01 '22 22:08 juvazq

I have simplified this PR to focus on the source of dependencies (lock vs toml) deferring the question of multiple registries to see if we can land the highest priority part this change.

danielframpton avatar Aug 03 '22 22:08 danielframpton

@danielframpton are the verification tests for rust still sufficient, or do they need updating as well?

JamieMagee avatar Aug 04 '22 16:08 JamieMagee

Snapshot tests are expected to fail due to changes in the detector.

JamieMagee avatar Aug 04 '22 17:08 JamieMagee

@danielframpton Do you have time to address the comments and resolve conflicts?

JamieMagee avatar Aug 26 '22 16:08 JamieMagee

@danielframpton Do we believe this PR is on track for September?

katshup avatar Sep 12 '22 15:09 katshup

@danielframpton thanks for deconflicting this PR ❤️

I just wanted to double check with you that you're good with this being merged?

JamieMagee avatar Oct 03 '22 22:10 JamieMagee

Thanks @JamieMagee!

I am, I have responded to all the feedback and updated across the code analysis fixes. Let me know if there is anything I have missed.

danielframpton avatar Oct 04 '22 03:10 danielframpton

👋 Hi! It looks like you modified some files in the Detectors folder. You may need to bump the detector versions if any of the following scenarios apply:

  • The detector detects more or fewer components than before
  • The detector generates different parent/child graph relationships than before
  • The detector generates different devDependencies values than before

If none of the above scenarios apply, feel free to ignore this comment 🙂

github-actions[bot] avatar Oct 05 '22 17:10 github-actions[bot]