Copilot commented on code in PR #255:
URL: https://github.com/apache/skywalking-eyes/pull/255#discussion_r2635578327
##########
pkg/deps/ruby.go:
##########
@@ -253,6 +398,183 @@ 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")
+ // 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
+ }
+ 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
+ }
+ // Ensure the character after the gem name
corresponds to the start of a version
+ if ver == "" || ver[0] < '0' || ver[0] > '9' {
+ 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 {
+ // 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.
+ license = strings.Join(licenses, " OR ")
+ }
+ }
+ }
+ if name != "" && license != "" {
+ break
+ }
+ }
Review Comment:
The function should check and return any scanner error that occurred during
file parsing. Currently, if scanner.Err() returns an error, it is silently
ignored and the function returns nil as the error. This could lead to
incomplete or incorrect data being returned without any indication that an
error occurred during parsing.
```suggestion
}
if err := scanner.Err(); err != nil {
return "", "", err
}
```
##########
pkg/deps/ruby.go:
##########
@@ -253,6 +398,183 @@ 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")
+ // 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
+ }
+ 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
+ }
+ // Ensure the character after the gem name
corresponds to the start of a version
+ if ver == "" || ver[0] < '0' || ver[0] > '9' {
+ 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 {
+ // 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.
+ license = strings.Join(licenses, " OR ")
Review Comment:
The comment explains that multiple licenses are joined with "OR",
acknowledging that some gems may intend "AND" instead. While the current
behavior is documented, it may not accurately represent the gem author's
intent. Consider adding a log warning when multiple licenses are detected, or
checking if the license array semantics can be determined from Ruby gem
conventions or metadata.
##########
pkg/deps/ruby.go:
##########
@@ -136,68 +243,114 @@ 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
Review Comment:
The currentRemotePath should be reset when entering a PATH block to prevent
potential state leakage between multiple PATH blocks. While every PATH block in
a well-formed Gemfile.lock should have a remote line, defensive programming
suggests explicitly resetting this field. Add `s.currentRemotePath = ""` to
prevent the remote path from a previous PATH block from being incorrectly
inherited if the current PATH block is missing a remote line.
```suggestion
s.inPath = true
s.currentRemotePath = ""
```
--
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]
