Copilot commented on code in PR #252:
URL: https://github.com/apache/skywalking-eyes/pull/252#discussion_r2517067778
##########
pkg/deps/npm.go:
##########
@@ -26,13 +26,55 @@ import (
"os"
"os/exec"
"path/filepath"
+ "regexp"
+ "runtime"
"strings"
"time"
"github.com/apache/skywalking-eyes/internal/logger"
"github.com/apache/skywalking-eyes/pkg/license"
)
+// Cross-platform package pattern recognition (for precise matching)
Review Comment:
[nitpick] The regex patterns match package names ending with platform
suffixes (using `$`), but npm packages with scopes use the format
`@scope/package-name`. The patterns should work correctly for scoped packages,
but it would be beneficial to add a comment clarifying that these patterns work
for both scoped and non-scoped packages.
Example to clarify:
```go
// Cross-platform package pattern recognition (for precise matching)
// These patterns work for both scoped (@scope/package-platform-arch) and
// non-scoped (package-platform-arch) npm packages
var platformPatterns = []struct {
```
```suggestion
// Cross-platform package pattern recognition (for precise matching)
// These patterns work for both scoped (@scope/package-platform-arch) and
// non-scoped (package-platform-arch) npm packages, as the platform/arch
// suffix always appears at the end of the package name.
// Examples:
// - Scoped: @scope/foo-linux-x64
// - Non-scoped: foo-linux-x64
```
##########
pkg/deps/npm.go:
##########
@@ -318,3 +369,67 @@ func (resolver *NpmResolver) ParsePkgFile(pkgFile string)
(*Package, error) {
}
return &packageInfo, nil
}
+
+// isForCurrentPlatform checks whether the given package name targets
+// the current platform. This method should be called before parsing
+// to determine if a package is cross-platform and not for the current
+// OS/architecture, so it can be safely skipped.
+func (resolver *NpmResolver) isForCurrentPlatform(pkgName string) bool {
+ pkgPlatform, pkgArch := resolver.analyzePackagePlatform(pkgName)
+ if pkgPlatform == "" && pkgArch == "" {
+ return true
+ }
+
+ currentOS := runtime.GOOS
+ currentArch := runtime.GOARCH
+
+ return pkgPlatform == currentOS && resolver.isArchCompatible(pkgArch,
currentArch)
+}
+
+// analyzePackagePlatform extracts the target operating system and architecture
+// from a package name by matching it against predefined regex patterns
+// (platformPatterns). Returns empty strings if no match is found.
+func (resolver *NpmResolver) analyzePackagePlatform(pkgName string) (string,
string) {
+ for _, pattern := range platformPatterns {
+ if pattern.regex.MatchString(pkgName) {
+ return pattern.os, pattern.arch
+ }
+ }
+ return "", ""
+}
+
+// isArchCompatible determines whether two architectures are considered
compatible.
+// For example, "x64" is compatible with "amd64", and "arm" is compatible with
"armv7".
+// Returns true if the architectures match or belong to the same compatibility
group.
+func (resolver *NpmResolver) isArchCompatible(pkgArch, currentArch string)
bool {
+ archCompatibility := map[string][]string{
+ "x64": {"x64", "x86_64", "amd64"},
+ "ia32": {"ia32", "x86", "386", "i386"},
+ "arm64": {"arm64", "aarch64"},
+ "arm": {"arm", "armv7", "armhf", "armv7l"},
+ "x86_64": {"x86_64", "x64", "amd64"},
+ "amd64": {"amd64", "x64", "x86_64"},
+ }
+
+ if pkgArch == currentArch {
+ return true
+ }
+
+ if compatibleArches, exists := archCompatibility[pkgArch]; exists {
+ for _, arch := range compatibleArches {
+ if arch == currentArch {
+ return true
+ }
+ }
+ }
+
+ if compatibleArches, exists := archCompatibility[currentArch]; exists {
+ for _, arch := range compatibleArches {
+ if arch == pkgArch {
+ return true
+ }
+ }
+ }
+
+ return false
Review Comment:
The architecture compatibility map contains duplicate entries for "x64",
"x86_64", and "amd64" that all map to the same architectures. This creates
redundancy in the lookup logic.
Consider simplifying the map to avoid duplication:
```go
archCompatibility := map[string][]string{
"x64": {"x64", "x86_64", "amd64"},
"ia32": {"ia32", "x86", "386", "i386"},
"arm64": {"arm64", "aarch64"},
"arm": {"arm", "armv7", "armhf", "armv7l"},
}
```
Then update the lookup logic to check both directions (pkgArch ->
currentArch and currentArch -> pkgArch) by normalizing both to a canonical form
first.
```suggestion
// Map all known aliases to a canonical architecture name
archCanonical := map[string]string{
"x64": "x64",
"x86_64": "x64",
"amd64": "x64",
"ia32": "ia32",
"x86": "ia32",
"386": "ia32",
"i386": "ia32",
"arm64": "arm64",
"aarch64": "arm64",
"arm": "arm",
"armv7": "arm",
"armhf": "arm",
"armv7l": "arm",
}
normalize := func(arch string) string {
if canon, ok := archCanonical[strings.ToLower(arch)]; ok {
return canon
}
return strings.ToLower(arch)
}
canonPkgArch := normalize(pkgArch)
canonCurrentArch := normalize(currentArch)
return canonPkgArch == canonCurrentArch
```
##########
pkg/deps/result.go:
##########
@@ -38,6 +38,7 @@ type Result struct {
LicenseSpdxID string
ResolveErrors []error
Version string
+ SkippedReason string
Review Comment:
[nitpick] The `SkippedReason` field is added to the `Result` struct but is
never used in the `Report.String()` method that generates the output table.
This means the skip reason won't be visible to users in the final report.
Consider displaying the skip reason in the output for skipped dependencies:
```go
for _, r := range report.Skipped {
reason := ""
if r.SkippedReason != "" {
reason = fmt.Sprintf(" (%s)", r.SkippedReason)
}
s += fmt.Sprintf(rowTemplate, r.Dependency + reason, Unknown, r.Version)
}
```
Alternatively, if the skip reason shouldn't appear in the table output,
consider documenting why this field is needed and where it's intended to be
used.
##########
pkg/deps/npm.go:
##########
@@ -87,6 +129,8 @@ func (resolver *NpmResolver) Resolve(pkgFile string, config
*ConfigDeps, report
for _, pkg := range pkgs {
if result := resolver.ResolvePackageLicense(pkg.Name, pkg.Path,
config); result.LicenseSpdxID != "" {
report.Resolve(result)
+ } else if result.SkippedReason != "" {
+ logger.Log.Warnf("Skipping cross-platform package %s
(not for current platform %s)", pkg.Name, runtime.GOOS)
Review Comment:
[nitpick] Platform-specific packages that are skipped are not tracked in the
report at all. This means they won't appear in the final output, and users may
not be aware that these packages were encountered and skipped.
Consider adding these skipped packages to the report:
```go
} else if result.SkippedReason != "" {
logger.Log.Warnf("Skipping cross-platform package %s (not for current
platform %s)", pkg.Name, runtime.GOOS)
// Add to skipped report so users can see these packages
// report.Skip(result) // or a new category for platform-specific skips
```
This would provide better visibility into which platform-specific
dependencies exist in the project.
```suggestion
logger.Log.Warnf("Skipping cross-platform package %s
(not for current platform %s)", pkg.Name, runtime.GOOS)
report.Skip(result)
```
--
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]
