Change Rust/Cargo detector to be lock file based #117

danielframpton · 2022-05-08T23:14:05Z

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

arlosi

I'm not an expert in CG, but I think this makes sense to get Cargo support to stop missing dependencies as mentioned in #116 - even if it means we give up dev-dependency detection for now.

src/Microsoft.ComponentDetection.Contracts/TypedComponent/CargoComponent.cs

src/Microsoft.ComponentDetection.Detectors/rust/Contracts/CargoPackage.cs

src/Microsoft.ComponentDetection.Detectors/rust/RustCrateDetector.cs

juvazq · 2022-07-29T21:54:58Z

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!

arlosi · 2022-07-29T21:58:55Z

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

juvazq · 2022-08-01T22:14:05Z

Thanks @arlosi,

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

danielframpton · 2022-08-03T22:39:33Z

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.

JamieMagee · 2022-08-04T16:13:17Z

src/Microsoft.ComponentDetection.Detectors/rust/RustCrateDetector.cs

+                        if (packagesByName.TryGetValue(cargoPackage.name, out var packageList))
+                        {
+                            if (packageList.Any(p => p.package.Equals(cargoPackage)))
+                            {
+                                // Ignore duplicate packages
+                                continue;
+                            }
+                        }


Should there be an else here? Or can these two conditionals be combined?

JamieMagee · 2022-08-04T16:13:40Z

src/Microsoft.ComponentDetection.Detectors/rust/RustCrateDetector.cs

+        {
+            try
+            {
+                // Extract the informationfrom the dependency (name with optional version and source)


Suggested change

// Extract the informationfrom the dependency (name with optional version and source)

// Extract the information from the dependency (name with optional version and source)

JamieMagee · 2022-08-04T16:17:17Z

src/Microsoft.ComponentDetection.Detectors/rust/RustCrateDetector.cs

+           @"^([^ ]+)(?: ([^ ]+))?(?: \(([^()]*)\))?$",
+           RegexOptions.Compiled);
+
+        private const int PackageNameGroup = 1;
+        private const int VersionGroup = 2;
+        private const int SourceGroup = 3;
+
+        private static bool ParseDependency(string dependency, out string packageName, out string version, out string source)
+        {
+            var match = DependencyFormatRegex.Match(dependency);
+            packageName = match.Groups[PackageNameGroup].Success ? match.Groups[PackageNameGroup].Value : null;
+            version = match.Groups[VersionGroup].Success ? match.Groups[VersionGroup].Value : null;
+            source = match.Groups[SourceGroup].Success ? match.Groups[SourceGroup].Value : null;


nit: named groups¹

Suggested change

@"^([^ ]+)(?: ([^ ]+))?(?: $([^()]*)$)?$",

RegexOptions.Compiled);

private const int PackageNameGroup = 1;

private const int VersionGroup = 2;

private const int SourceGroup = 3;

private static bool ParseDependency(string dependency, out string packageName, out string version, out string source)

{

var match = DependencyFormatRegex.Match(dependency);

packageName = match.Groups[PackageNameGroup].Success ? match.Groups[PackageNameGroup].Value : null;

version = match.Groups[VersionGroup].Success ? match.Groups[VersionGroup].Value : null;

source = match.Groups[SourceGroup].Success ? match.Groups[SourceGroup].Value : null;

@"^(?<packageName>[^ ]+)(?: (?<version>[^ ]+))?(?: $(?<source>[^()]*)$)?$",

RegexOptions.Compiled);

private static bool ParseDependency(string dependency, out string packageName, out string version, out string source)

{

var match = DependencyFormatRegex.Match(dependency);

packageName = match.Groups["packageName"].Success ? match.Groups["packageName"].Value : null;

version = match.Groups["version"].Success ? match.Groups["version"].Value : null;

source = match.Groups["source"].Success ? match.Groups["source"].Value : null;

Footnotes

https://docs.microsoft.com/en-us/dotnet/standard/base-types/grouping-constructs-in-regular-expressions#named_matched_subexpression ↩

JamieMagee · 2022-08-04T16:20:15Z

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

JamieMagee

Approved pending comments

JamieMagee · 2022-08-04T17:36:38Z

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

JamieMagee · 2022-08-26T16:44:29Z

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

grvillic · 2022-08-30T20:59:35Z

src/Microsoft.ComponentDetection.Detectors/rust/RustCrateDetector.cs

+                            if (!IsLocalPackage(package) && !seenAsDependency.Contains(package))
+                            {
+                                var detectedComponent = new DetectedComponent(component);
+                                singleFileComponentRecorder.RegisterUsage(detectedComponent, isExplicitReferencedDependency: true);


What happened to dev dependency detection? Is that being considered in these updates, if so, how will those be treated now? Users won't like to start picking up dependencies as regular build time dependencies when they used to be dev dependencies.

This does not report dev dependencies separately (e.g., they are included alongside other dependencies as well) as that information is not currently included in the lock file. Unfortunately, there isn't an obvious middle ground here between having something that is simple and robust (lock file processing) or replicating the full functionality of cargo.

This change puts us in the position of not missing dependencies, with the option to (in the future) introduce a tool that uses cargo to get a more accurate picture (either directly or by consuming SBOMs generated from cargo).

katshup · 2022-09-12T15:38:59Z

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

Review feedback Revert inclusion of registry in package identity for the initial change PR feedback and adding a multiple registry test (to validate that it doesn't cause problems)

JamieMagee · 2022-10-03T22:47:59Z

@danielframpton thanks for deconflicting this PR ❤️

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

danielframpton · 2022-10-04T03:31:54Z

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.

github-actions · 2022-10-05T17:19:54Z

👋 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 🙂

danielframpton requested a review from a team as a code owner May 8, 2022 23:14

danielframpton requested a review from jcfiorenzano May 8, 2022 23:14

arlosi reviewed May 9, 2022

View reviewed changes

src/Microsoft.ComponentDetection.Detectors/rust/RustCrateDetector.cs Outdated Show resolved Hide resolved

cobya added the detector:pip The pip detector label May 16, 2022

jcfiorenzano added detector:rust The Rust Cargo detector and removed detector:pip The pip detector labels May 27, 2022

JamieMagee requested review from JamieMagee and cobya June 15, 2022 17:09

danielframpton force-pushed the cargo-fixes branch 2 times, most recently from ad8b3f2 to 42ba5f2 Compare June 16, 2022 19:04

danielframpton force-pushed the cargo-fixes branch from 42ba5f2 to d8a3cbc Compare August 3, 2022 22:37

JamieMagee reviewed Aug 4, 2022

View reviewed changes

JamieMagee approved these changes Aug 4, 2022

View reviewed changes

arlosi approved these changes Aug 4, 2022

View reviewed changes

grvillic reviewed Aug 30, 2022

View reviewed changes

danielframpton force-pushed the cargo-fixes branch from d8a3cbc to 45fc653 Compare September 30, 2022 21:56

Change Rust/Cargo detector to be lock file based.

ae9ca2f

Review feedback Revert inclusion of registry in package identity for the initial change PR feedback and adding a multiple registry test (to validate that it doesn't cause problems)

danielframpton force-pushed the cargo-fixes branch from 45fc653 to ae9ca2f Compare October 1, 2022 02:23

JamieMagee added the version:minor label Oct 5, 2022

Merge branch 'main' into cargo-fixes

1e69eef

JamieMagee enabled auto-merge (squash) October 5, 2022 17:01

JamieMagee merged commit c7c4ce8 into microsoft:main Oct 5, 2022

JamieMagee mentioned this pull request Oct 21, 2022

Rust detector silently discards non-dev dependencies when Cargo.toml file contains dependency from git #253

Closed

tofay mentioned this pull request Jan 30, 2024

remove category usage from the poetry detector #991

Merged

	// Extract the informationfrom the dependency (name with optional version and source)
	// Extract the information from the dependency (name with optional version and source)

Change Rust/Cargo detector to be lock file based #117

Change Rust/Cargo detector to be lock file based #117

Uh oh!

Conversation

danielframpton commented May 8, 2022

Uh oh!

arlosi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juvazq commented Jul 29, 2022

Uh oh!

arlosi commented Jul 29, 2022

Uh oh!

juvazq commented Aug 1, 2022

Uh oh!

danielframpton commented Aug 3, 2022

Uh oh!

JamieMagee Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

JamieMagee Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

JamieMagee Aug 4, 2022

Choose a reason for hiding this comment

Footnotes

Uh oh!

JamieMagee commented Aug 4, 2022

Uh oh!

JamieMagee left a comment

Choose a reason for hiding this comment

Uh oh!

JamieMagee commented Aug 4, 2022

Uh oh!

JamieMagee commented Aug 26, 2022

Uh oh!

grvillic Aug 30, 2022

Choose a reason for hiding this comment

Uh oh!

danielframpton Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

katshup commented Sep 12, 2022

Uh oh!

JamieMagee commented Oct 3, 2022

Uh oh!

danielframpton commented Oct 4, 2022

Uh oh!

github-actions bot commented Oct 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants