Copilot commented on code in PR #255:
URL: https://github.com/apache/skywalking-eyes/pull/255#discussion_r2634796339
##########
pkg/deps/ruby_test.go:
##########
@@ -117,6 +117,81 @@ func TestRubyGemfileLockResolver(t *testing.T) {
t.Fatalf("expected 1 dependency for library, got %d",
len(report.Resolved)+len(report.Skipped))
}
}
+
+ // Citrus case: library with gemspec, no runtime deps
+ {
+ tmp := t.TempDir()
+ if err := copyRuby("testdata/ruby/citrus", tmp); err != nil {
+ t.Fatal(err)
+ }
+ lock := filepath.Join(tmp, "Gemfile.lock")
+ if !resolver.CanResolve(lock) {
+ t.Fatalf("GemfileLockResolver cannot resolve %s", lock)
+ }
+ cfg := &ConfigDeps{Files: []string{lock}}
+ report := Report{}
+ if err := resolver.Resolve(lock, cfg, &report); err != nil {
+ t.Fatal(err)
+ }
+ // Should have 0 dependencies because citrus has no runtime deps
+ if len(report.Resolved)+len(report.Skipped) != 0 {
+ t.Fatalf("expected 0 dependencies, got %d",
len(report.Resolved)+len(report.Skipped))
+ }
+ }
+
+ // Local dependency case: App depends on local gem (citrus)
+ {
+ tmp := t.TempDir()
+ if err := copyRuby("testdata/ruby/local_dep", tmp); err != nil {
+ t.Fatal(err)
+ }
+ lock := filepath.Join(tmp, "Gemfile.lock")
+ if !resolver.CanResolve(lock) {
+ t.Fatalf("GemfileLockResolver cannot resolve %s", lock)
+ }
+ cfg := &ConfigDeps{Files: []string{lock}}
+ report := Report{}
+ if err := resolver.Resolve(lock, cfg, &report); err != nil {
+ t.Fatal(err)
+ }
+
+ // We expect citrus to be resolved with MIT license.
+ // Currently it fails (Unknown) because of missing PATH support
and ! handling.
Review Comment:
This comment states "Currently it fails (Unknown)" but the code has been
fixed to support local path dependencies. The comment should be updated to
reflect that this test now validates that the fix works correctly.
##########
pkg/deps/ruby.go:
##########
@@ -253,6 +367,142 @@ func runtimeDepsFromGemspecs(dir string) ([]string,
error) {
return res, nil
}
+func parseGemspecDependencies(path string) ([]string, error) {
+ f, err := os.Open(path)
+ if err != nil {
+ return nil, err
+ }
+ defer f.Close()
+
+ scanner := bufio.NewScanner(f)
+ var deps []string
+ for scanner.Scan() {
+ line := scanner.Text()
+ trimLeft := strings.TrimLeft(line, " \t")
+ if strings.HasPrefix(trimLeft, "#") {
+ continue
+ }
+ if m := gemspecRuntimeRe.FindStringSubmatch(line); len(m) == 2 {
+ deps = append(deps, m[1])
+ }
+ }
+ return deps, scanner.Err()
+}
+
+func findInstalledGemspec(name, version string) (string, error) {
+ gemPaths := getGemPaths()
+ for _, dir := range gemPaths {
+ specsDir := filepath.Join(dir, "specifications")
+ if version != "" && !strings.ContainsAny(version, "<>~=") {
+ path := filepath.Join(specsDir,
name+"-"+version+".gemspec")
+ if _, err := os.Stat(path); err == nil {
+ return path, nil
+ }
+ } else {
+ entries, err := os.ReadDir(specsDir)
+ if err != nil {
+ continue
+ }
+ for _, e := range entries {
+ if !e.IsDir() && strings.HasPrefix(e.Name(),
name+"-") && strings.HasSuffix(e.Name(), ".gemspec") {
+ stem := strings.TrimSuffix(e.Name(),
".gemspec")
+ ver := strings.TrimPrefix(stem,
name+"-")
+ if ver == stem {
+ continue
+ }
+ return filepath.Join(specsDir,
e.Name()), nil
+ }
+ }
+ }
+ }
+ return "", os.ErrNotExist
+}
+
+func fetchLocalLicense(dir, name string) (string, error) {
Review Comment:
The name parameter is not used in the function body. If it's intended for
future use or validation, consider adding a comment explaining why it's
present. Otherwise, consider removing it or using it to validate that the found
gemspec matches the expected gem name.
##########
pkg/deps/ruby.go:
##########
@@ -138,19 +240,30 @@ func parseGemfileLock(s string) (graph gemGraph, roots
[]string, err error) {
inSpecs := false
inDeps := false
+ inPath := false
var current *gemSpec
+ var currentRemotePath string
for scanner.Scan() {
line := scanner.Text()
if strings.HasPrefix(line, "GEM") {
inSpecs = true
inDeps = false
+ inPath = false
+ current = nil
+ continue
Review Comment:
When transitioning to a GEM block, the currentRemotePath variable is not
reset. This means if a GEM block follows a PATH block, and the GEM block's
remote is not captured (which it shouldn't be for GEM blocks), gems in the GEM
section could incorrectly inherit the LocalPath from the previous PATH block.
Add currentRemotePath = "" after setting inPath = false to ensure clean state
transitions.
##########
pkg/deps/ruby_test.go:
##########
@@ -117,6 +117,81 @@ func TestRubyGemfileLockResolver(t *testing.T) {
t.Fatalf("expected 1 dependency for library, got %d",
len(report.Resolved)+len(report.Skipped))
}
}
+
+ // Citrus case: library with gemspec, no runtime deps
+ {
+ tmp := t.TempDir()
+ if err := copyRuby("testdata/ruby/citrus", tmp); err != nil {
+ t.Fatal(err)
+ }
+ lock := filepath.Join(tmp, "Gemfile.lock")
+ if !resolver.CanResolve(lock) {
+ t.Fatalf("GemfileLockResolver cannot resolve %s", lock)
+ }
+ cfg := &ConfigDeps{Files: []string{lock}}
+ report := Report{}
+ if err := resolver.Resolve(lock, cfg, &report); err != nil {
+ t.Fatal(err)
+ }
+ // Should have 0 dependencies because citrus has no runtime deps
+ if len(report.Resolved)+len(report.Skipped) != 0 {
+ t.Fatalf("expected 0 dependencies, got %d",
len(report.Resolved)+len(report.Skipped))
+ }
+ }
+
+ // Local dependency case: App depends on local gem (citrus)
+ {
+ tmp := t.TempDir()
+ if err := copyRuby("testdata/ruby/local_dep", tmp); err != nil {
+ t.Fatal(err)
+ }
+ lock := filepath.Join(tmp, "Gemfile.lock")
+ if !resolver.CanResolve(lock) {
+ t.Fatalf("GemfileLockResolver cannot resolve %s", lock)
+ }
+ cfg := &ConfigDeps{Files: []string{lock}}
+ report := Report{}
+ if err := resolver.Resolve(lock, cfg, &report); err != nil {
+ t.Fatal(err)
+ }
+
+ // We expect citrus to be resolved with MIT license.
+ // Currently it fails (Unknown) because of missing PATH support
and ! handling.
+ found := false
+ for _, r := range report.Resolved {
+ if r.Dependency == "citrus" {
+ found = true
+ if r.LicenseSpdxID != "MIT" {
+ t.Errorf("expected MIT license for
citrus, got %s", r.LicenseSpdxID)
+ }
+ }
+ }
+ // Also check Skipped if it failed
+ for _, r := range report.Skipped {
+ if strings.HasPrefix(r.Dependency, "citrus") {
+ // This is where it currently lands
Review Comment:
This comment states "This is where it currently lands" but the fix has been
implemented to prevent citrus from landing in Skipped with Unknown license. The
comment should be updated to reflect that the test validates the fix works
correctly, or the logic should be simplified to only check report.Resolved.
```suggestion
// Historically citrus landed here; keep this
check to ensure it no longer appears in Skipped with Unknown license.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
