feat: add new target riscv64gc-unknown-linux-musl#96
Merged
Conversation
|
""" WalkthroughThe changes update the build and packaging process to add support for the Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub Actions
participant Build Environment
Developer->>GitHub Actions: Push code / trigger release workflow
GitHub Actions->>Build Environment: Start build job (no version check gating)
Build Environment->>Build Environment: Build for riscv64gc-unknown-linux-musl
Build Environment->>GitHub Actions: Upload build artifacts
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new build target for riscv64gc-unknown-linux-musl and temporarily removes the condition that checks for version changes during the build process.
- Commented out a condition that restricts builds based on version changes.
- Added a new build configuration for the riscv64gc-unknown-linux-musl target.
Files not reviewed (1)
- npm/package.json: Language not supported
Comments suppressed due to low confidence (1)
.github/workflows/release-napi.yml:45
- Commenting out the version check condition may cause builds to run even when no version change has occurred. Confirm if this removal is intentional or consider re-adding a conditional check.
# if: needs.check.outputs.version_changed == 'true'
JounQin
commented
Apr 24, 2025
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 153d870 in 40 seconds. Click for details.
- Reviewed
40lines of code in2files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release-napi.yml:45
- Draft comment:
The version check condition is commented out. Confirm if builds should always run or if this was temporary. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm their intention regarding a commented-out version check condition. This violates the rule against asking the author to confirm their intention or ensure behavior is intended. Therefore, this comment should be removed.
2. .github/workflows/release-napi.yml:90
- Draft comment:
New target for riscv64gc-unknown-linux-musl is added. Verify that 'gcc-riscv64-linux-musl' packages are available on ubuntu-latest and the build command matches expectations. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. npm/package.json:40
- Draft comment:
New target 'riscv64gc-unknown-linux-musl' added in the napi.targets array. Confirm ordering and consistency with other target entries. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. .github/workflows/release-napi.yml:45
- Draft comment:
The 'if:' condition is commented out. Confirm if this is intentional to always trigger builds; if not, re-enable the condition. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm their intention regarding a commented-out condition. This violates the rule against asking the author to confirm their intention or ensure behavior is intended. The comment should be removed.
5. .github/workflows/release-napi.yml:90
- Draft comment:
New matrix entry for 'riscv64gc-unknown-linux-musl' added with its own cross-compiler setup. Ensure the apt packages (gcc/g++ for riscv64-linux-musl) are available and correct. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that the apt packages are available and correct, which violates the rule against asking the author to ensure things. It doesn't provide a specific suggestion or point out a specific issue with the code.
6. npm/package.json:40
- Draft comment:
Added 'riscv64gc-unknown-linux-musl' to the NAPI targets. This should resolve the EBADPLATFORM issue for riscv64 systems; please verify the build output behaves as expected. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is informative and asks the PR author to verify the build output, which violates the rules. It doesn't provide a specific code suggestion or ask for a specific test to be written.
Workflow ID: wflow_CwIXuE6qSmskOUVi
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 7250ca3 in 1 minute and 6 seconds. Click for details.
- Reviewed
28lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release-napi.yml:93
- Draft comment:
Ensure that using 'pnpm build -x' for the riscv64gc-unknown-linux-musl target properly configures the compiler and environment. This differs from the riscv64gc-unknown-linux-gnu target, which explicitly installs and sets CC/CXX. Verify that no additional setup is needed for MUSL. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to verify that the setup for a specific build target is correct. It is not making a specific code suggestion or pointing out a specific issue with the code. It falls under the category of asking the author to ensure behavior is intended, which is against the rules.
2. .github/workflows/release-napi.yml:10
- Draft comment:
Good addition: the new pull_request trigger ensures CI runs on PRs per common practice. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. .github/workflows/release-napi.yml:87
- Draft comment:
Using 'pnpm build -x' for riscv64gc-unknown-linux-musl now aligns with other musl targets. Ensure that any necessary cross-compiler setup (previously done via apt-get and env vars) isn’t required for a correct build. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
Workflow ID: wflow_C8UutUiG8RQfbtXS
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed ce91401 in 46 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release-napi.yml:37
- Draft comment:
Ensure the new URL (https://app.unpkg.com/unrs-resolver@latest/files/package.json) reliably returns the expected package.json. This change should be validated to avoid potential version mismatches or broken content. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. .github/workflows/release-napi.yml:37
- Draft comment:
Ensure the new file URL (https://app.unpkg.com/unrs-resolver@latest/files/package.json) returns the expected package.json structure for accurate version checking and isn’t affected by caching issues. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. .github/workflows/release-napi.yml:93
- Draft comment:
The new riscv64gc-unknown-linux-musl target uses 'pnpm build -x', similar to other musl targets. Confirm that no additional cross-compilation tooling is needed for this target. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_dRl31ZNeYy4AWlYC
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
ce91401 to
997f373
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 997f373 in 39 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release-napi.yml:37
- Draft comment:
Ensure switching the file-url from jsDelivr to unpkg is intentional. Verify that the package.json from unpkg reliably reflects the latest version, especially since it affects version-check for new targets. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. .github/workflows/release-napi.yml:37
- Draft comment:
The file-url has been updated to use unpkg.com instead of cdn.jsdelivr.net. Consider pinning a specific version instead of using '@latest' for consistent behavior. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_aGowNLgAifuuEgL4
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
JounQin
commented
Apr 24, 2025
|
JounQin
commented
Apr 24, 2025
| id: version | ||
| with: | ||
| static-checking: localIsNew | ||
| file-url: https://cdn.jsdelivr.net/npm/unrs-resolver@latest/package.json |
This was referenced Apr 24, 2025
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close #95
Important
Add
riscv64gc-unknown-linux-musltarget and update release workflow to always build.riscv64gc-unknown-linux-musltarget to build matrix inrelease-napi.ymlandpackage.json.release-napi.ymlto always run the build process by commenting out the version check condition.This description was created by
for 997f373. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Chores