Copilot commented on code in PR #255:
URL: https://github.com/apache/skywalking-eyes/pull/255#discussion_r2635022809
##########
pkg/deps/ruby.go:
##########
@@ -136,68 +240,111 @@ func parseGemfileLock(s string) (graph gemGraph, roots
[]string, err error) {
scanner.Split(bufio.ScanLines)
graph = make(gemGraph)
- inSpecs := false
- inDeps := false
- var current *gemSpec
+ state := &lockParserState{
+ graph: graph,
+ }
for scanner.Scan() {
- line := scanner.Text()
- if strings.HasPrefix(line, "GEM") {
- inSpecs = true
- inDeps = false
- current = nil
- continue
- }
- if strings.HasPrefix(line, "DEPENDENCIES") {
- inSpecs = false
- inDeps = true
- current = nil
- continue
- }
- if strings.TrimSpace(line) == "specs:" && inSpecs {
- // just a marker
- continue
- }
+ state.processLine(scanner.Text())
+ }
+ if err := scanner.Err(); err != nil {
+ return nil, nil, err
+ }
+ return graph, state.roots, nil
+}
- if inSpecs {
- if m := lockSpecHeader.FindStringSubmatch(line); len(m)
== 3 {
- name := m[1]
- version := m[2]
- current = &gemSpec{Name: name, Version: version}
- graph[name] = current
- continue
- }
- if current != nil {
- if m := lockDepLine.FindStringSubmatch(line);
len(m) == 2 {
- depName := m[1]
- current.Deps = append(current.Deps,
depName)
- }
- }
- continue
+type lockParserState struct {
+ inSpecs bool
+ inDeps bool
+ inPath bool
+ current *gemSpec
+ currentRemotePath string
+ graph gemGraph
+ roots []string
+}
+
+func (s *lockParserState) processLine(line string) {
+ if strings.HasPrefix(line, "GEM") {
+ s.inSpecs = true
+ s.inDeps = false
+ s.inPath = false
+ s.currentRemotePath = ""
+ s.current = nil
+ return
+ }
+ if strings.HasPrefix(line, "PATH") {
+ s.inSpecs = true
+ s.inDeps = false
+ s.inPath = true
+ s.current = nil
+ return
+ }
+ if strings.HasPrefix(line, "DEPENDENCIES") {
+ s.inSpecs = false
+ s.inDeps = true
+ s.inPath = false
+ s.current = nil
+ return
+ }
+ if strings.TrimSpace(line) == "specs:" && s.inSpecs {
+ // just a marker
+ return
+ }
+
+ if s.inSpecs {
+ s.processSpecs(line)
+ return
+ }
+
+ if s.inDeps {
+ s.processDeps(line)
+ return
+ }
+}
+
+func (s *lockParserState) processSpecs(line string) {
+ trim := strings.TrimSpace(line)
+ if strings.HasPrefix(trim, "remote:") {
+ if s.inPath {
+ s.currentRemotePath =
strings.TrimSpace(strings.TrimPrefix(trim, "remote:"))
}
Review Comment:
Consider adding a comment explaining that the `inPath` check ensures that
only PATH block remote paths are captured, not GEM block remote URLs (like
https://rubygems.org/). This distinction is important for proper local
dependency resolution.
##########
pkg/deps/ruby.go:
##########
@@ -253,6 +390,157 @@ 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") {
Review Comment:
The condition checking `strings.HasPrefix(e.Name(), name+"-")` could produce
false positives when gem names are substrings of other gem names. For example,
searching for "foo" would also match "foo-bar-1.0.0.gemspec". Consider using a
more precise check that validates the character after the name prefix is a dash
followed by a version number, or matches the exact pattern
"name-version.gemspec".
##########
pkg/deps/ruby.go:
##########
@@ -214,6 +361,8 @@ func hasGemspec(dir string) bool {
}
var gemspecRuntimeRe =
regexp.MustCompile(`\badd_(?:runtime_)?dependency\s*\(?\s*["']([^"']+)["']`)
Review Comment:
The regex pattern `\.licenses?\s*=\s*(?:\[\s*)?['"]([^'"]+)['"]` only
captures the first license when multiple licenses are specified in an array
(e.g., `s.licenses = ['MIT', 'Apache-2.0']`). The pattern should be improved to
handle multiple licenses in arrays or at least document this limitation.
Consider capturing all licenses within array brackets or using a different
parsing approach for arrays.
```suggestion
var gemspecRuntimeRe =
regexp.MustCompile(`\badd_(?:runtime_)?dependency\s*\(?\s*["']([^"']+)["']`)
// Note: this pattern only captures the first license when multiple licenses
are specified in an array.
```
##########
pkg/deps/ruby.go:
##########
@@ -253,6 +390,157 @@ 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, "<>~=") {
Review Comment:
The version constraint check `!strings.ContainsAny(version, "<>~=")` is
incomplete. Ruby version constraints can also use operators like `!=` (not
equal) and `.pre` suffixes for pre-release versions. A version string like
"1.0.0.pre" would incorrectly be treated as a specific version. Consider using
a more robust check or regex pattern to validate version numbers.
```suggestion
if version != "" && !strings.ContainsAny(version, "<>~=!") &&
!strings.Contains(version, ".pre") {
```
##########
pkg/deps/ruby.go:
##########
@@ -253,6 +390,157 @@ 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, targetName string) (string, error) {
+ entries, err := os.ReadDir(dir)
+ if err != nil {
+ return "", err
+ }
+ for _, e := range entries {
+ if e.IsDir() || !strings.HasSuffix(e.Name(), ".gemspec") {
+ continue
+ }
+ path := filepath.Join(dir, e.Name())
+ specName, license, err := parseGemspecInfo(path)
+ if err == nil && specName == targetName && license != "" {
+ return license, nil
+ }
+ }
+ return "", nil
+}
+
+func fetchInstalledLicense(name, version string) string {
+ gemPaths := getGemPaths()
+ for _, dir := range gemPaths {
+ specsDir := filepath.Join(dir, "specifications")
+ // If version is specific
+ if version != "" && !strings.ContainsAny(version, "<>~=") { //
simple check if it's a version number
Review Comment:
The version constraint check `!strings.ContainsAny(version, "<>~=")` is
incomplete. Ruby version constraints can also use operators like `!=` (not
equal) and `.pre` suffixes for pre-release versions. A version string like
"1.0.0.pre" would incorrectly be treated as a specific version. Consider using
a more robust check or regex pattern to validate version numbers.
##########
pkg/deps/ruby.go:
##########
@@ -253,6 +390,157 @@ 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, targetName string) (string, error) {
+ entries, err := os.ReadDir(dir)
+ if err != nil {
+ return "", err
+ }
+ for _, e := range entries {
+ if e.IsDir() || !strings.HasSuffix(e.Name(), ".gemspec") {
+ continue
+ }
+ path := filepath.Join(dir, e.Name())
+ specName, license, err := parseGemspecInfo(path)
+ if err == nil && specName == targetName && license != "" {
+ return license, nil
+ }
+ }
+ return "", nil
+}
+
+func fetchInstalledLicense(name, version string) string {
+ gemPaths := getGemPaths()
+ for _, dir := range gemPaths {
+ specsDir := filepath.Join(dir, "specifications")
+ // If version is specific
+ if version != "" && !strings.ContainsAny(version, "<>~=") { //
simple check if it's a version number
+ path := filepath.Join(specsDir,
name+"-"+version+".gemspec")
+ if _, license, err := parseGemspecInfo(path); err ==
nil && license != "" {
+ return license
+ }
+ } else {
+ // Scan for any version
+ 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") {
+ continue
+ }
+ stem := strings.TrimSuffix(e.Name(), ".gemspec")
+ ver := strings.TrimPrefix(stem, name+"-")
+ if ver == stem { // didn't have prefix
+ continue
+ }
+ path := filepath.Join(specsDir, e.Name())
+ if _, license, err := parseGemspecInfo(path);
err == nil && license != "" {
+ return license
+ }
+ }
+ }
+ }
+ return ""
+}
+
+func getGemPaths() []string {
+ env := os.Getenv("GEM_PATH")
+ if env == "" {
+ env = os.Getenv("GEM_HOME")
+ }
+ if env == "" {
+ return nil
+ }
+ return strings.Split(env, string(os.PathListSeparator))
+}
+
+func parseGemspecInfo(path string) (string, string, error) {
+ f, err := os.Open(path)
+ if err != nil {
+ return "", "", err
+ }
+ defer f.Close()
+ scanner := bufio.NewScanner(f)
+ var name, license string
+ for scanner.Scan() {
+ line := scanner.Text()
+ trimLeft := strings.TrimLeft(line, " \t")
+ if strings.HasPrefix(trimLeft, "#") {
+ continue
+ }
+ if name == "" {
+ if m := gemspecNameRe.FindStringSubmatch(line); len(m)
== 2 {
+ name = m[1]
+ }
+ }
+ if license == "" {
+ if m := gemspecLicenseRe.FindStringSubmatch(line);
len(m) == 2 {
+ license = m[1]
+ }
+ }
+ if name != "" && license != "" {
+ break
+ }
+ }
+ return name, license, nil
+}
+
+func parseGemspecLicense(path string) (string, error) {
+ _, license, err := parseGemspecInfo(path)
+ return license, err
+}
+
Review Comment:
The function `parseGemspecLicense` appears to be unused and simply wraps
`parseGemspecInfo`. Consider removing this function if it's not used elsewhere
in the codebase to reduce code duplication.
```suggestion
```
##########
pkg/deps/ruby.go:
##########
@@ -116,12 +131,101 @@ func (r *GemfileLockResolver) Resolve(lockfile string,
config *ConfigDeps, repor
return nil
}
+// GemspecResolver resolves dependencies from a .gemspec file.
+type GemspecResolver struct {
+ Resolver
+}
Review Comment:
The GemspecResolver struct and its Resolve method lack documentation
comments explaining their purpose and behavior. Consider adding a doc comment
describing that this resolver handles .gemspec files by extracting runtime
dependencies and recursively resolving their transitive dependencies from
installed gems.
##########
pkg/deps/ruby.go:
##########
@@ -116,12 +131,101 @@ func (r *GemfileLockResolver) Resolve(lockfile string,
config *ConfigDeps, repor
return nil
}
+// GemspecResolver resolves dependencies from a .gemspec file.
+type GemspecResolver struct {
+ Resolver
+}
+
+func (r *GemspecResolver) CanResolve(file string) bool {
+ return strings.HasSuffix(file, ".gemspec")
+}
+
+func (r *GemspecResolver) Resolve(file string, config *ConfigDeps, report
*Report) error {
+ f, err := os.Open(file)
+ if err != nil {
+ return err
+ }
+ defer f.Close()
+
+ scanner := bufio.NewScanner(f)
+ deps := make(map[string]string) // name -> version constraint
+ 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[m[1]] = "" // We don't extract version constraint
yet, or we could improve regex
+ }
+ }
+ if err := scanner.Err(); err != nil {
+ return err
+ }
+
+ // Recursive resolution
+ queue := make([]string, 0, len(deps))
+ for name := range deps {
+ queue = append(queue, name)
+ }
+
+ visited := make(map[string]struct{})
+ for name := range deps {
Review Comment:
The queue is initialized with all direct dependencies before the BFS loop
begins. Consider removing the separate initialization loops at lines 167-174
and instead initialize the queue and visited map in a single loop to reduce
code duplication.
```suggestion
visited := make(map[string]struct{}, len(deps))
for name := range deps {
queue = append(queue, name)
```
##########
pkg/deps/ruby.go:
##########
@@ -253,6 +390,157 @@ 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, targetName string) (string, error) {
+ entries, err := os.ReadDir(dir)
+ if err != nil {
+ return "", err
+ }
+ for _, e := range entries {
+ if e.IsDir() || !strings.HasSuffix(e.Name(), ".gemspec") {
+ continue
+ }
+ path := filepath.Join(dir, e.Name())
+ specName, license, err := parseGemspecInfo(path)
+ if err == nil && specName == targetName && license != "" {
+ return license, nil
+ }
+ }
+ return "", nil
+}
+
+func fetchInstalledLicense(name, version string) string {
+ gemPaths := getGemPaths()
+ for _, dir := range gemPaths {
+ specsDir := filepath.Join(dir, "specifications")
+ // If version is specific
+ if version != "" && !strings.ContainsAny(version, "<>~=") { //
simple check if it's a version number
+ path := filepath.Join(specsDir,
name+"-"+version+".gemspec")
+ if _, license, err := parseGemspecInfo(path); err ==
nil && license != "" {
+ return license
+ }
+ } else {
+ // Scan for any version
+ 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") {
Review Comment:
The condition checking `strings.HasPrefix(e.Name(), name+"-")` could produce
false positives when gem names are substrings of other gem names. For example,
searching for "foo" would also match "foo-bar-1.0.0.gemspec". Consider using a
more precise check that validates the character after the name prefix is a dash
followed by a version number, or matches the exact pattern
"name-version.gemspec".
```suggestion
re := regexp.MustCompile("^" + regexp.QuoteMeta(name) +
"-[0-9].*\\.gemspec$")
for _, e := range entries {
if e.IsDir() || !re.MatchString(e.Name()) {
```
--
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]
