Copilot commented on code in PR #255:
URL: https://github.com/apache/skywalking-eyes/pull/255#discussion_r2635225405
##########
pkg/deps/ruby.go:
##########
@@ -214,36 +367,27 @@ func hasGemspec(dir string) bool {
}
var gemspecRuntimeRe =
regexp.MustCompile(`\badd_(?:runtime_)?dependency\s*\(?\s*["']([^"']+)["']`)
+var gemspecLicenseRe = regexp.MustCompile(`\.licenses?\s*=\s*(.*)`)
+var gemspecStringRe = regexp.MustCompile(`['"]([^'"]+)['"]`)
+var gemspecNameRe = regexp.MustCompile(`\.name\s*=\s*['"]([^'"]+)['"]`)
+var rubyVersionRe =
regexp.MustCompile(`^\d+(\.[0-9a-zA-Z]+)*(-[0-9a-zA-Z]+)?$`)
func runtimeDepsFromGemspecs(dir string) ([]string, error) {
entries, err := os.ReadDir(dir)
if err != nil {
return nil, err
- }
- runtime := make(map[string]struct{})
+ } runtime := make(map[string]struct{})
for _, e := range entries {
if e.IsDir() || !strings.HasSuffix(e.Name(), ".gemspec") {
Review Comment:
Missing newline after closing brace. The line `} runtime :=
make(map[string]struct{})` should have the `runtime` declaration on a new line
for better readability and to follow Go formatting conventions.
```suggestion
}
runtime := make(map[string]struct{})
for _, e := range entries {
```
##########
pkg/deps/ruby.go:
##########
@@ -214,36 +367,27 @@ func hasGemspec(dir string) bool {
}
var gemspecRuntimeRe =
regexp.MustCompile(`\badd_(?:runtime_)?dependency\s*\(?\s*["']([^"']+)["']`)
+var gemspecLicenseRe = regexp.MustCompile(`\.licenses?\s*=\s*(.*)`)
Review Comment:
The regex pattern `\.licenses?\s*=\s*(.*)` is overly greedy and will capture
everything after the equals sign to the end of the line, including comments.
This could lead to incorrect parsing if there's a comment on the same line.
Consider using a more specific pattern that handles common gemspec license
declarations, such as capturing array syntax or quoted strings more precisely.
```suggestion
var gemspecLicenseRe = regexp.MustCompile(`\.licenses?\s*=\s*([^#]*)`)
```
##########
pkg/deps/ruby.go:
##########
@@ -253,6 +397,165 @@ 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 != "" && rubyVersionRe.MatchString(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") {
+ continue
+ }
+ stem := strings.TrimSuffix(e.Name(), ".gemspec")
Review Comment:
The prefix matching logic could incorrectly match gems with similar names.
For example, when searching for gem "foo", it would match
"foo-bar-1.0.0.gemspec" even though the gem name is "foo-bar", not "foo". While
there's a validation at line 446 using `parseGemspecInfo`, this still causes
unnecessary file parsing. Consider adding a check to ensure the character after
the gem name in the filename is a hyphen followed by a digit (start of
version), to avoid false matches.
```suggestion
stem := strings.TrimSuffix(e.Name(), ".gemspec")
// Ensure that the character immediately after
the "name-" prefix
// is a digit, so we only consider filenames
where the suffix is
// a version component (e.g.,
"foo-1.0.0.gemspec") and avoid
// similar names like "foo-bar-1.0.0.gemspec"
when searching for "foo".
if len(stem) <= len(name)+1 {
continue
}
versionStart := stem[len(name)+1]
if versionStart < '0' || versionStart > '9' {
continue
}
```
##########
pkg/deps/ruby.go:
##########
@@ -253,6 +397,165 @@ 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 != "" && rubyVersionRe.MatchString(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") {
+ continue
+ }
+ stem := strings.TrimSuffix(e.Name(), ".gemspec")
+ ver := strings.TrimPrefix(stem, name+"-")
+ if ver == stem {
+ continue
+ }
+ path := filepath.Join(specsDir, e.Name())
+ if specName, _, err := parseGemspecInfo(path);
err == nil && specName == name {
+ return path, 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 != "" && rubyVersionRe.MatchString(version) {
+ 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
+ }
Review Comment:
The prefix matching logic could incorrectly match gems with similar names.
For example, when searching for gem "foo", it would match
"foo-bar-1.0.0.gemspec" even though the gem name is "foo-bar", not "foo". While
there's a validation at line 499 using `parseGemspecInfo`, this still causes
unnecessary file parsing. Consider adding a check to ensure the character after
the gem name in the filename is a hyphen followed by a digit (start of
version), to avoid false matches.
```suggestion
}
// Ensure the character after the gem name
corresponds to the start of a version
if len(ver) == 0 || ver[0] < '0' || ver[0] >
'9' {
continue
}
```
##########
pkg/deps/ruby.go:
##########
@@ -253,6 +397,165 @@ 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 != "" && rubyVersionRe.MatchString(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") {
+ continue
+ }
+ stem := strings.TrimSuffix(e.Name(), ".gemspec")
+ ver := strings.TrimPrefix(stem, name+"-")
+ if ver == stem {
+ continue
+ }
+ path := filepath.Join(specsDir, e.Name())
+ if specName, _, err := parseGemspecInfo(path);
err == nil && specName == name {
+ return path, 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 != "" && rubyVersionRe.MatchString(version) {
+ 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 specName, license, err :=
parseGemspecInfo(path); err == nil && specName == name && 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) (gemName, gemLicense string, err 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 {
+ matches :=
gemspecStringRe.FindAllStringSubmatch(m[1], -1)
+ var licenses []string
+ for _, match := range matches {
+ if len(match) == 2 {
+ licenses = append(licenses,
match[1])
+ }
+ }
+ if len(licenses) > 0 {
Review Comment:
Joining multiple licenses with " OR " may not be semantically correct. In
SPDX, "OR" indicates that the code can be used under any of the listed licenses
(disjunctive). However, some gems may declare multiple licenses that must all
be satisfied (conjunctive, using "AND"). The current implementation treats all
multi-license cases as disjunctive, which may misrepresent the actual licensing
terms. Consider documenting this assumption or adding logic to detect the
intended relationship between licenses.
```suggestion
if len(licenses) > 0 {
// NOTE: When multiple licenses are
declared in the gemspec, we assume they are
// alternatives and represent them with
SPDX-style "OR". Some gems may instead
// intend all listed licenses to apply
("AND"), which is not distinguished here.
```
--
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]
