-
Notifications
You must be signed in to change notification settings - Fork 64
Fix: Compatibility issue with Node.js 24 (related to apache/skywalking#13517) #252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Compatibility issue with Node.js 24 (related to apache/skywalking#13517) #252
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a compatibility issue with Node.js 24 where the make license-dep command fails due to incorrect handling of platform-specific npm packages. The solution adds intelligent platform detection to skip packages that are not compatible with the current OS/architecture before attempting license parsing.
Key Changes:
- Added platform-specific package detection using regex patterns for various OS/architecture combinations (Linux, macOS, Windows, FreeBSD, Android)
- Implemented architecture compatibility logic to handle variations like x64/amd64/x86_64
- Added
SkippedReasonfield to track why packages are skipped
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/deps/result.go | Added SkippedReason field to Result struct to track why packages are skipped during resolution |
| pkg/deps/npm.go | Core implementation: added platform pattern recognition (39 patterns), platform detection logic in isForCurrentPlatform, architecture compatibility checking in isArchCompatible, and updated resolution flow to skip non-matching platform packages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Please fix CI and take a look at Copilot review comments. |
|
Sorry for the long delay. I've completed the revisions. Could you please help rerun the CI and review the code? |
|
I fixed the CI errors. |
|
@kezhenxu94 Please take a look. @hanahmily I think you could build a local version, and have a try? |
kezhenxu94
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’d be better if you can add a test case
Co-authored-by: kezhenxu94 <kezhenxu94@apache.org>
Detect npm packages that declare an OS suffix without a CPU architecture (e.g. -linux, -darwin) and emit a friendly warning instead of treating them as universal packages.
Co-authored-by: ning666meng <ning666meng@users.noreply.github.com> Merge branch 'skipping-cross-platform-packages' of https://github.com/XL-Zhao-23/skywalking-eyes into skipping-cross-platform-packages
8057ebf to
0ed30bf
Compare
…/XL-Zhao-23/skywalking-eyes into skipping-cross-platform-packages
|
@XL-Zhao-23 Would you like to try this in BanyanDB repo to adopt this new feature? cc @hanahmily |
|
No problem, I will test it again. (I have tested it in BanyanDB, but it was before adding the test cases.) |
|
This is the validation result for the current version: added 218 packages, and audited 219 packages in 5s 52 packages are looking for funding found 0 vulnerabilities
|
|
👌 |
Background
This PR fixes the compatibility issue reported in apache/skywalking#13517,
where the
make license-depcommand fails under Node.js 24 due to incorrect handling of platform-specific npm packages.Root Cause
The SkyWalking Eyes tool failed to correctly recognize platform-specific npm packages
(e.g.,
@parcel/watcher-linux-x64-musl,@parcel/watcher-win32-arm64),resulting in license parsing errors under newer Node.js versions.
Solution
-linux-x64-musl$,-win32-arm64$).Verification
make license-depin thebanyandbproject.Related Issue
Fixes apache/skywalking#13517