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.

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

Add rust cargo support for dep command. #121

Merged
merged 6 commits into from
Jul 7, 2022
Merged

Add rust cargo support for dep command. #121

merged 6 commits into from
Jul 7, 2022

Conversation

jmjoy
Copy link
Member

@jmjoy jmjoy commented Jul 7, 2022

Add rust cargo support for dep command.

@wu-sheng wu-sheng requested a review from kezhenxu94 July 7, 2022 10:39
@wu-sheng wu-sheng added the enhancement New feature or request label Jul 7, 2022
@wu-sheng wu-sheng added this to the 0.4.0 milestone Jul 7, 2022
@kezhenxu94 kezhenxu94 added feature enhancement New feature or request and removed enhancement New feature or request labels Jul 7, 2022
Comment on lines 117 to 119
if pkg.License == "" {
return fmt.Errorf("license is empty")
}
Copy link
Member

@kezhenxu94 kezhenxu94 Jul 7, 2022

Choose a reason for hiding this comment

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

Should be this?

Suggested change
if pkg.License == "" {
return fmt.Errorf("license is empty")
}
if pkg.License != "" {
report.Resolve(&Result{
Dependency: pkg.Name,
LicenseSpdxID: pkg.License,
Version: pkg.Version,
})
return nil
}

If there is an explicit license ID, I think we can just use them

Copy link
Member Author

Choose a reason for hiding this comment

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

I wrote it with reference to GoModResolver, which returns an error, the calling place will warn and call report.Skip

Copy link
Member

@kezhenxu94 kezhenxu94 Jul 7, 2022

Choose a reason for hiding this comment

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

Hi @jmjoy , I mean if pkg.License != "" we can just use pkg.License (no need to read its license file content and just return here), if pkg.License == "" we will try to find its license file and identify the license ID by the license file content

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, but the --output generated files will all be empty if so.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kezhenxu94 In fact, I don't need the --output parameter, but I saw that this parameter originally existed, so I implemented it.

Copy link
Member

Choose a reason for hiding this comment

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

@jmjoy I think you might want to apply this patch

diff --git a/pkg/deps/cargo.go b/pkg/deps/cargo.go
index d1057ab..e2613d2 100644
--- a/pkg/deps/cargo.go
+++ b/pkg/deps/cargo.go
@@ -19,13 +19,13 @@ package deps
 
 import (
 	"encoding/json"
-	"fmt"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"regexp"
 
 	"github.com/apache/skywalking-eyes/internal/logger"
+	"github.com/apache/skywalking-eyes/pkg/license"
 )
 
 type CargoMetadata struct {
@@ -114,10 +114,6 @@ var cargoPossibleLicenseFileName = regexp.MustCompile(`(?i)^LICENSE|LICENCE(\.tx
 // ResolvePackageLicense resolve the package license.
 // The CargoPackage.LicenseFile is generally used for non-standard licenses and is ignored now.
 func (resolver *CargoTomlResolver) ResolvePackageLicense(config *ConfigDeps, pkg *CargoPackage, report *Report) error {
-	if pkg.License == "" {
-		return fmt.Errorf("license is empty")
-	}
-
 	dir := filepath.Dir(pkg.ManifestPath)
 	logger.Log.Debugf("Directory of %+v is %+v", pkg.Name, dir)
 	files, err := os.ReadDir(dir)
@@ -128,6 +124,8 @@ func (resolver *CargoTomlResolver) ResolvePackageLicense(config *ConfigDeps, pkg
 	var licenseFilePath string
 	var licenseContent []byte
 
+	licenseID := pkg.License
+
 	for _, info := range files {
 		if !cargoPossibleLicenseFileName.MatchString(info.Name()) {
 			continue
@@ -142,11 +140,17 @@ func (resolver *CargoTomlResolver) ResolvePackageLicense(config *ConfigDeps, pkg
 		break
 	}
 
+	if licenseID == "" { // If pkg.License is empty, identify the license ID from the license file content
+		if licenseID, err = license.Identify(string(licenseContent), config.Threshold); err != nil {
+			return err
+		}
+	}
+
 	report.Resolve(&Result{
 		Dependency:      pkg.Name,
 		LicenseFilePath: licenseFilePath,
 		LicenseContent:  string(licenseContent),
-		LicenseSpdxID:   pkg.License,
+		LicenseSpdxID:   licenseID,
 		Version:         pkg.Version,
 	})
 

Copy link
Member

Choose a reason for hiding this comment

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

The basic idea to identify the license is

  1. Obtain from the metadata of the package, if they already have the license id, we'd just use it, otherwise
  2. Try to find the license file in the package, and try to identify the license id from the license file content.

Copy link
Member Author

@jmjoy jmjoy Jul 7, 2022

Choose a reason for hiding this comment

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

Thanks, it's better, just push the patch to my branch? Because I open the Allow edits and access to secrets by maintainers.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, it's better, just push the patch to my branch? Because I open the Allow edits and access to secrets by maintainers.

Yes. Feel free to just git apply my patch and push in this PR. Or if you want me to edit I can do that too.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kezhenxu94 Done.

@wu-sheng wu-sheng merged commit 0cb30eb into apache:main Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants