feat: allow installing the TS library on node-alpine #1029
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to b30d5d2 in 19 seconds
More details
- Looked at
267lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_UuW5ycXxdaA6WCfm
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
|
||
| concurrency: | ||
| # suffix is important to prevent a concurrency deadlock with the calling workflow | ||
| group: ${{ github.workflow }}-${{ github.ref }}-build-python |
There was a problem hiding this comment.
The concurrency group name seems incorrect. It should be build-typescript instead of build-python to match the workflow's purpose.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 3b16424 in 13 seconds
More details
- Looked at
81lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. .github/workflows/build-typescript-release.reusable.yaml:60
- Draft comment:
Consider using 'host' instead of 'runs_on' for consistency with other targets. - Reason this comment was not posted:
Confidence changes required:50%
The 'runs_on' key is used instead of 'host' for the 'x86_64-unknown-linux-musl' target, which is inconsistent with other targets.
2. .github/workflows/build-typescript-release.reusable.yaml:56
- Draft comment:
Setting 'container' to 'null' might cause issues if a container is expected. Consider removing the key if no container is needed. - Reason this comment was not posted:
Confidence changes required:50%
The 'container' key is set to 'null' for 'x86_64-unknown-linux-gnu', which might cause issues if a container is expected.
Workflow ID: wflow_frue1KaCF5cNWfmD
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 4202542 in 12 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build-typescript-release.reusable.yaml:10
- Draft comment:
Concurrency group name updated tobuild-typescriptto match the TypeScript build process. This change is appropriate. - Reason this comment was not posted:
Confidence changes required:0%
The concurrency group name was updated frombuild-pythontobuild-typescript, which aligns with the TypeScript build process. This change is appropriate and necessary for the new workflow.
Workflow ID: wflow_kptsLg3mCITsBewe
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 9f0ad67 in 13 seconds
More details
- Looked at
34lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build-typescript-release.reusable.yaml:36
- Draft comment:
Ensure 'cargo_args' is defined for the current targets in the matrix to avoid potential errors during the Rust build step. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_HgprVX3yfIvK31c1
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on b0fb613 in 22 seconds
More details
- Looked at
30lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_Yem8HDKg2kSyBRSk
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
|
||
| - target: x86_64-unknown-linux-musl | ||
| host: node-alpine | ||
| node_build: apk add musl-dev gcc build-base && CARGO_TARGET_AARCH64_UNKNOWN_LINUX_MUSL_LINKER=aarch64-alpine-linux-musl-gcc pnpm build:napi-release --target x86_64-unknown-linux-musl --use-napi-cross |
There was a problem hiding this comment.
The environment variable CARGO_TARGET_AARCH64_UNKNOWN_LINUX_MUSL_LINKER is incorrectly used for the x86_64-unknown-linux-musl target. It should be CARGO_TARGET_X86_64_UNKNOWN_LINUX_MUSL_LINKER.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 5d8be1b in 20 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_1a74QF1HdQ79AH2g
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
|
||
| - target: x86_64-unknown-linux-musl | ||
| host: node-alpine | ||
| node_build: apk add musl-dev gcc build-base && CARGO_TARGET_X86_64_UNKNOWN_LINUX_MUSL_LINKER=aarch64-alpine-linux-musl-gcc pnpm build:napi-release --target x86_64-unknown-linux-musl --use-napi-cross |
There was a problem hiding this comment.
The linker environment variable CARGO_TARGET_X86_64_UNKNOWN_LINUX_MUSL_LINKER is incorrectly set to aarch64-alpine-linux-musl-gcc. It should be x86_64-alpine-linux-musl-gcc to match the target architecture.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 63460f2 in 21 seconds
More details
- Looked at
36lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build-typescript-release.reusable.yaml:112
- Draft comment:
Consider making thebeforestep conditional to avoid errors when it's not defined for some matrix entries. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is addressing a potential issue where the 'before' step is not defined for all matrix entries, which could cause errors during execution. This is a valid concern as the 'before' step is only defined for specific targets in the matrix. Making it conditional would prevent errors when the step is not needed.
The comment assumes that the absence of the 'before' step will cause errors, but it's possible that the workflow handles undefined steps gracefully. Without testing or more context, it's speculative.
However, the suggestion to make the step conditional is a common practice to prevent potential errors, and it is a straightforward change that improves robustness.
Keep the comment as it addresses a valid concern about potential errors due to undefined 'before' steps in the matrix.
Workflow ID: wflow_noa8ymqxEOxd79Fm
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 9f1027d in 11 seconds
More details
- Looked at
31lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build-typescript-release.reusable.yaml:60
- Draft comment:
The tar command is extracting 'x86_64-linux-musl-native.tgz', but the curl command downloads 'x86_64-linux-musl-cross.tgz'. Ensure the file names match to avoid errors. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_JwOfoNs0kmekARAH
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 466c9dc in 20 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build-typescript-release.reusable.yaml:59
- Draft comment:
The URL change fromx86_64-linux-musl-cross.tgztox86_64-linux-musl-native.tgzmight be incorrect if the intent was to use a cross-compiler. Thenativeversion is typically for native builds, not cross-compilation. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment addresses a specific change in the diff, which is the URL modification. It provides a rationale that 'native' is typically used for native builds, not cross-compilation, which could be a valid concern if the intention was indeed to use a cross-compiler. This is a potential code logic issue, not just a stylistic or informative comment.
The comment is speculative, as it uses the word 'might', which suggests uncertainty. Without additional context or confirmation of the intended use, it's difficult to determine if this is a definite issue.
The comment is based on a common understanding of the terms 'native' and 'cross' in the context of compilers, which could indicate a potential issue if the change was unintentional.
The comment should be kept as it highlights a potential issue with the change from 'cross' to 'native', which could affect the build process if the intention was to use a cross-compiler.
Workflow ID: wflow_YfwoDaLO6fL7kZfK
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 9a13cf1 in 27 seconds
More details
- Looked at
24lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build-typescript-release.reusable.yaml:67
- Draft comment:
The environment variableCARGO_TARGET_X86_64_UNKNOWN_LINUX_MUSL_LINKERshould usex86_64-linux-musl-gccinstead ofx86_64-linux-musl-ccfor consistency with other variables. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment points out an inconsistency in the naming of the linker variable compared to other variables in the same block. This could be a valid point if consistency is important for the build process. However, without more context on whether 'x86_64-linux-musl-cc' is functionally different from 'x86_64-linux-musl-gcc', it's hard to determine if this is a necessary change.
I might be missing the specific role of 'x86_64-linux-musl-cc' in the build process. It could be that 'x86_64-linux-musl-cc' is the correct tool for the job, and changing it to 'x86_64-linux-musl-gcc' might introduce issues.
The comment is about consistency, which is generally a good practice in code. However, without knowing the specific requirements of the build process, it's speculative to assume that the change is necessary.
Delete the comment because it is speculative and lacks strong evidence that the change is necessary.
Workflow ID: wflow_0KQbk6qZbFVaeQ5I
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on f5721cf in 11 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build-typescript-release.reusable.yaml:67
- Draft comment:
Unnecessary whitespace change. Ensure consistent formatting across the file. - Reason this comment was not posted:
Confidence changes required:10%
The change is a minor whitespace adjustment, which is not significant. However, it's good to ensure consistency in formatting.
Workflow ID: wflow_IqI2rcyBzDCKoNra
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 997179c in 10 seconds
More details
- Looked at
59lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. .github/workflows/build-typescript-release.reusable.yaml:68
- Draft comment:
Remove commented-out environment variable settings if they are not needed. - Reason this comment was not posted:
Confidence changes required:50%
The commented-out environment variable settings for musl targets should be removed if not needed.
2. engine/language_client_typescript/Cargo.toml:14
- Draft comment:
TheCargo.tomlfile is correctly configured for musl targets. - Reason this comment was not posted:
Confidence changes required:0%
TheCargo.tomlfile seems to be correctly configured for the musl targets.
Workflow ID: wflow_MD8juZ5ulWw7PhHx
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on a609923 in 13 seconds
More details
- Looked at
124lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build-typescript-release.reusable.yaml:84
- Draft comment:
Ensurepnpmis installed before using it. Thepnpm/action-setup@v3action was removed, butpnpmis still used in the workflow. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_1nfSyHru6xtUz4hP
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on ea0d8e2 in 20 seconds
More details
- Looked at
57lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build-typescript-release.reusable.yaml:86
- Draft comment:
Theifcondition should beif: matrix.miseinstead ofif: matrix._.mise. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The matrix is defined with an underscore key, which is unconventional. The comment suggests a change that might be necessary if the underscore is not intended. However, without more context on whether the underscore is a mistake or intentional, it's hard to definitively say the comment is correct. The comment is speculative without strong evidence that the current code is incorrect.
I might be missing the context of whether the underscore in the matrix key is intentional or a mistake. The comment assumes it's a mistake without strong evidence.
Given the unconventional use of the underscore, it's reasonable to question its correctness, but without strong evidence, the comment remains speculative.
The comment is speculative and lacks strong evidence that the current code is incorrect. It should be deleted as it does not meet the criteria for a necessary code change.
Workflow ID: wflow_oE7DvfrSfHoTcipm
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 2e1558d in 29 seconds
More details
- Looked at
106lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_3QKkkRexRwFW3gP3
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| tar -xzf aarch64-linux-musl-cross.tgz | ||
| echo "$PWD/aarch64-linux-musl-cross/bin" >> $GITHUB_PATH | ||
| node_build: pnpm build:napi-release --target aarch64-unknown-linux-musl --use-napi-cross | ||
|
|
There was a problem hiding this comment.
The before script for aarch64-unknown-linux-musl target is missing the environment variable setup for the compiler tools. Consider adding the following:
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on a997679 in 15 seconds
More details
- Looked at
37lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build-typescript-release.reusable.yaml:73
- Draft comment:
Ensure all matrix entries have acontainervalue if thecontainerkey is used in the job configuration. This prevents potential errors when the workflow runs. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_XtKq69X7IBwEB0jN
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
sxlijin
changed the title
There was a problem hiding this comment.
❌ Changes requested. Incremental review on e714e07 in 20 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_akD1Rh2SoWtLlemg
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -2,6 +2,8 @@ name: BAML Release | |||
|
|
|||
| on: | |||
| workflow_dispatch: {} | |||
| push: | |||
| branches: [sam/alpine-builds] | |||
There was a problem hiding this comment.
The 'push' event is defined twice. Consider merging them to avoid unexpected behavior.
sxlijin
force-pushed
the
sam/alpine-builds
branch
from
b65ea84 to
aa50754
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on b65ea84 in 21 seconds
More details
- Looked at
67lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. tools/setup-cross-compile-env.sh:32
- Draft comment:
It's recommended to update the package list before installing packages withapt. Consider uncommenting theapt-get updateline beforeapt install. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is about a change in the diff, specifically the commented-out 'apt-get update' line. Updating the package list is a standard practice and can prevent issues during package installation. This seems like a valid suggestion for improving the script's reliability.
The script might be intended to run in a controlled environment where the package list is already up-to-date, making the update unnecessary. Without more context, it's hard to be certain.
Even in controlled environments, it's generally safer to update the package list to avoid potential issues. The suggestion is a good practice and doesn't harm the script's functionality.
The comment is valid and suggests a good practice that can prevent potential issues. It should be kept.
2. tools/setup-cross-compile-env.sh:60
- Draft comment:
Ensure thatbashis available in the environment, as the script ends with starting a bash shell. Consider adding a check or installing it if necessary. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_xN4yuAmPcB7uAuxm
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on aa50754 in 22 seconds
More details
- Looked at
67lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. tools/setup-cross-compile-env.sh:32
- Draft comment:
It's recommended to update the package list before installing packages withapt. Consider uncommenting theapt-get updateline beforeapt install. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is about a change in the script where 'apt-get update' is commented out. Updating the package list before installation is a standard practice to avoid issues with outdated package information. This seems like a valid suggestion for a code change.
The script might be intended to run in a controlled environment where the package list is already up-to-date, making the update unnecessary. Without more context, it's hard to be certain.
Even in controlled environments, it's generally safer to update the package list to prevent potential issues. The suggestion aligns with best practices.
The comment is valid as it suggests a best practice change to uncomment 'apt-get update'. It should be kept.
2. tools/setup-cross-compile-env.sh:60
- Draft comment:
Ensurebashis available in the environment, or consider usingshifbashis not necessary. - Reason this comment was not posted:
Comment did not seem useful.
3. tools/setup-cross-compile-env.sh:52
- Draft comment:
Modifying~/.bashrcassumes the user will use an interactive bash shell. Document this assumption or consider alternative ways to ensure the environment is set up correctly. - Reason this comment was not posted:
Confidence changes required:50%
The script modifies~/.bashrc, which might not be sourced if the shell is not interactive or if the user uses a different shell. Consider documenting this behavior.
Workflow ID: wflow_ksH0VO53oU8futBY
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Modularizes TypeScript build process with a reusable GitHub Actions workflow and updates Cargo and Node.js configurations for musl support.
build-typescript-release.reusable.yamlfor TypeScript builds supporting targets likex86_64-unknown-linux-gnuandx86_64-unknown-linux-musl.release.ymlto use the new reusable workflow for TypeScript builds.Cargo.tomlto specify linkers forx86_64-unknown-linux-muslandaarch64-unknown-linux-musltargets..mise.tomlto20.14.setup-cross-compile-env.shscript for setting up cross-compilation environment in Docker.This description was created by
for aa50754. It will automatically update as commits are pushed.