Add build scritps for Windows, macOs and Linux#109
Open
peacefulprogram wants to merge 3 commits intoHarlonWang:mainfrom
Open
Add build scritps for Windows, macOs and Linux#109peacefulprogram wants to merge 3 commits intoHarlonWang:mainfrom
peacefulprogram wants to merge 3 commits intoHarlonWang:mainfrom
Conversation
Reviewer's Guide添加跨平台 JNI 构建脚本和文档，修复一个 QuickJS 类定义初始化器，并调整 CMake include 路径以更好地支持 Windows 构建。 File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your Experience访问你的 dashboard 以：
Getting HelpOriginal review guide in EnglishReviewer's GuideAdds cross-platform JNI build scripts and documentation, fixes a QuickJS class definition initializer, and adjusts CMake include paths to better support Windows builds. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了 6 个问题，并留下了一些整体性的反馈：
- 在 quickjs_wrapper.cpp 中，将 JSClassDef 从指定初始化器切换为位置参数，会让代码在上游结构体变更时更加脆弱；建议保留指定初始化器，或者显式将结构体剩余字段置零初始化。
- CMakeLists.txt 中硬编码的
$ENV{JAVA_HOME}/include/win32路径在所有 JDK 发行版上可能并不存在（有些使用windows）；可以考虑自动检测正确的子目录，或者按平台做条件包含。 - README 中的构建说明和新的 build-jni 脚本现在存在逻辑重复；建议更新 README，让其直接引用这些脚本，以避免未来两边内容出现偏差。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- 在 quickjs_wrapper.cpp 中，将 JSClassDef 从指定初始化器切换为位置参数，会让代码在上游结构体变更时更加脆弱；建议保留指定初始化器，或者显式将结构体剩余字段置零初始化。
- CMakeLists.txt 中硬编码的 `$ENV{JAVA_HOME}/include/win32` 路径在所有 JDK 发行版上可能并不存在（有些使用 `windows`）；可以考虑自动检测正确的子目录，或者按平台做条件包含。
- README 中的构建说明和新的 build-jni 脚本现在存在逻辑重复；建议更新 README，让其直接引用这些脚本，以避免未来两边内容出现偏差。
## Individual Comments
### Comment 1
<location path="build-jni.sh" line_range="1-3" />
<code_context>
+
+- For Linux and macOS
+```shell
+cd wrapper-java/src/main
+cmake -B build -S . -DCMAKE_BUILD_TYPE=Release
+cmake --build build
</code_context>
<issue_to_address>
**suggestion:** 使用相对路径的 `cd` 会导致脚本在仓库根目录之外运行时变得脆弱
这依赖当前工作目录是仓库根目录，如果在其他位置调用就会出错。建议从脚本所在位置推导路径，比如 `cd "$(dirname "$0")/wrapper-java/src/main"`，这样无论从哪里调用都能正常工作。
```suggestion
#!/bin/bash
set -e
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
cd "$SCRIPT_DIR/wrapper-java/src/main"
```
</issue_to_address>
### Comment 2
<location path="build-jni.sh" line_range="16" />
<code_context>
+else
+ DYLIB_EXT="so" # Linux
+fi
+cp build/libquickjs-java-wrapper.$DYLIB_EXT ../../..
+echo Build success
\ No newline at end of file
</code_context>
<issue_to_address>
**suggestion:** `cp` 中未加引号的路径在包含空格时会出问题
如果仓库所在路径中有空格（例如位于 `Program Files` 下），这个 `cp` 会失败。请同时给源路径和目标路径加上引号，例如 `cp "build/libquickjs-java-wrapper.$DYLIB_EXT" "../../.."`，以提高在此类路径下的健壮性。
```suggestion
cp "build/libquickjs-java-wrapper.$DYLIB_EXT" "../../.."
```
</issue_to_address>
### Comment 3
<location path="build-jni.bat" line_range="2" />
<code_context>
+For Windows users, you should install [mingw-w64](https://github.com/skeeto/w64devkit) and add **bin** directory to **PATH**.
+- For Windows
+```bash
+cd wrapper-java\src\main
+cmake -B build -S . -G "MinGW Makefiles" -DCMAKE_BUILD_TYPE=Release
+cmake --build build
</code_context>
<issue_to_address>
**suggestion:** 批处理脚本中的相对 `cd` 和未加引号路径在某些环境下会导致失败
这些命令假定脚本从仓库根目录运行，并且目录路径中没有空格。为了提高健壮性，建议基于脚本所在路径构造路径（例如 `cd /d "%~dp0wrapper-java\src\main"`），并为所有路径参数加上引号。
</issue_to_address>
### Comment 4
<location path="build-jni.bat" line_range="7-8" />
<code_context>
+ echo Deleting old build folder...
+ rmdir /s /q build
+)
+cmake -B build -S . -G "MinGW Makefiles" -DCMAKE_BUILD_TYPE=Release
+cmake --build build
+COPY build\libquickjs-java-wrapper.dll ..\..\..
+echo Build success
</code_context>
<issue_to_address>
**suggestion (bug_risk):** 批处理脚本缺少错误处理，可能会掩盖构建失败
如果某条 `cmake` 命令失败，脚本仍然会复制 DLL 并打印 `Build success`，从而掩盖失败。请在每次 `cmake` 调用之后增加错误检查（例如 `if errorlevel 1 goto :eof` 或 `exit /b 1`），这样在构建失败时脚本会停止执行并返回非零退出码。
</issue_to_address>
### Comment 5
<location path="README.md" line_range="75" />
<code_context>
git submodule update --init
```
+To build the JNI shared library, please follow these steps:
+- Install [jdk](https://docs.aws.amazon.com/corretto/latest/corretto-17-ug/downloads-list.html) and [cmake](https://cmake.org/download/).
+- Set the **JAVA_HOME** system environment variable, and add **cmake** to your **PATH**.
+For Windows users, you should install [mingw-w64](https://github.com/skeeto/w64devkit) and add **bin** directory to **PATH**.
</code_context>
<issue_to_address>
**nitpick (typo):** 建议将链接文本中的 "jdk" 首字母大写为 "JDK"。
由于这里指的是 Java Development Kit 的产品名称，请在链接文本中使用 "JDK" 而不是 "jdk"。
```suggestion
- Install [JDK](https://docs.aws.amazon.com/corretto/latest/corretto-17-ug/downloads-list.html) and [cmake](https://cmake.org/download/).
```
</issue_to_address>
### Comment 6
<location path="README.md" line_range="77" />
<code_context>
+To build the JNI shared library, please follow these steps:
+- Install [jdk](https://docs.aws.amazon.com/corretto/latest/corretto-17-ug/downloads-list.html) and [cmake](https://cmake.org/download/).
+- Set the **JAVA_HOME** system environment variable, and add **cmake** to your **PATH**.
+For Windows users, you should install [mingw-w64](https://github.com/skeeto/w64devkit) and add **bin** directory to **PATH**.
+- For Windows
+```bash
</code_context>
<issue_to_address>
**suggestion (typo):** 关于将 bin 目录加入 PATH 的语法可以再更严谨一些。
可以改成：“For Windows users, install [mingw-w64](...) and add the **bin** directory to your **PATH**.” 这样补上了缺失的冠词，并更明确地说明是添加到用户自己的 PATH 中。
```suggestion
For Windows users, install [mingw-w64](https://github.com/skeeto/w64devkit) and add the **bin** directory to your **PATH**.
```
</issue_to_address>帮我变得更有用！请在每条评论上点击 👍 或 👎，我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've found 6 issues, and left some high level feedback:
- In quickjs_wrapper.cpp, switching from a designated initializer to positional arguments for JSClassDef makes the code more brittle against upstream struct changes; consider keeping designated initializers or explicitly zero-initializing the rest of the struct.
- The hardcoded
$ENV{JAVA_HOME}/include/win32path in CMakeLists.txt may not exist on all JDK distributions (some usewindowsinstead); consider detecting the correct subdirectory or guarding this include by platform. - The README build instructions and the new build-jni scripts now duplicate logic; consider updating the README to reference the scripts directly to avoid divergence in future changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In quickjs_wrapper.cpp, switching from a designated initializer to positional arguments for JSClassDef makes the code more brittle against upstream struct changes; consider keeping designated initializers or explicitly zero-initializing the rest of the struct.
- The hardcoded `$ENV{JAVA_HOME}/include/win32` path in CMakeLists.txt may not exist on all JDK distributions (some use `windows` instead); consider detecting the correct subdirectory or guarding this include by platform.
- The README build instructions and the new build-jni scripts now duplicate logic; consider updating the README to reference the scripts directly to avoid divergence in future changes.
## Individual Comments
### Comment 1
<location path="build-jni.sh" line_range="1-3" />
<code_context>
+
+- For Linux and macOS
+```shell
+cd wrapper-java/src/main
+cmake -B build -S . -DCMAKE_BUILD_TYPE=Release
+cmake --build build
</code_context>
<issue_to_address>
**suggestion:** Using a relative `cd` makes the script fragile when run from outside the repo root
This relies on the current working directory being the repo root, so it breaks when invoked from elsewhere. Prefer deriving the path from the script location, e.g. `cd "$(dirname "$0")/wrapper-java/src/main"`, so it works regardless of where it’s called from.
```suggestion
#!/bin/bash
set -e
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
cd "$SCRIPT_DIR/wrapper-java/src/main"
```
</issue_to_address>
### Comment 2
<location path="build-jni.sh" line_range="16" />
<code_context>
+else
+ DYLIB_EXT="so" # Linux
+fi
+cp build/libquickjs-java-wrapper.$DYLIB_EXT ../../..
+echo Build success
\ No newline at end of file
</code_context>
<issue_to_address>
**suggestion:** Unquoted paths in `cp` can break when the path contains spaces
If the repo is in a path with spaces (e.g. under `Program Files`), this `cp` will fail. Please quote both source and destination, e.g. `cp "build/libquickjs-java-wrapper.$DYLIB_EXT" "../../.."`, to make it robust to such paths.
```suggestion
cp "build/libquickjs-java-wrapper.$DYLIB_EXT" "../../.."
```
</issue_to_address>
### Comment 3
<location path="build-jni.bat" line_range="2" />
<code_context>
+For Windows users, you should install [mingw-w64](https://github.com/skeeto/w64devkit) and add **bin** directory to **PATH**.
+- For Windows
+```bash
+cd wrapper-java\src\main
+cmake -B build -S . -G "MinGW Makefiles" -DCMAKE_BUILD_TYPE=Release
+cmake --build build
</code_context>
<issue_to_address>
**suggestion:** Relative `cd` and unquoted paths in the batch script can cause failures in some environments
These commands assume the script is run from the repo root and that no directories contain spaces. To make it more robust, base paths on the script location (e.g. `cd /d "%~dp0wrapper-java\src\main"`) and quote all path arguments.
</issue_to_address>
### Comment 4
<location path="build-jni.bat" line_range="7-8" />
<code_context>
+ echo Deleting old build folder...
+ rmdir /s /q build
+)
+cmake -B build -S . -G "MinGW Makefiles" -DCMAKE_BUILD_TYPE=Release
+cmake --build build
+COPY build\libquickjs-java-wrapper.dll ..\..\..
+echo Build success
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Lack of error handling in the batch script can hide build failures
If a `cmake` command fails, the script still copies the DLL and prints `Build success`, masking the failure. Add an error check (e.g., `if errorlevel 1 goto :eof` or `exit /b 1`) after each `cmake` call so the script stops and returns a non‑zero code on build failure.
</issue_to_address>
### Comment 5
<location path="README.md" line_range="75" />
<code_context>
git submodule update --init
```
+To build the JNI shared library, please follow these steps:
+- Install [jdk](https://docs.aws.amazon.com/corretto/latest/corretto-17-ug/downloads-list.html) and [cmake](https://cmake.org/download/).
+- Set the **JAVA_HOME** system environment variable, and add **cmake** to your **PATH**.
+For Windows users, you should install [mingw-w64](https://github.com/skeeto/w64devkit) and add **bin** directory to **PATH**.
</code_context>
<issue_to_address>
**nitpick (typo):** Consider capitalizing "jdk" to "JDK" in the link text.
Because this refers to the Java Development Kit product name, please use "JDK" rather than "jdk" in the link text.
```suggestion
- Install [JDK](https://docs.aws.amazon.com/corretto/latest/corretto-17-ug/downloads-list.html) and [cmake](https://cmake.org/download/).
```
</issue_to_address>
### Comment 6
<location path="README.md" line_range="77" />
<code_context>
+To build the JNI shared library, please follow these steps:
+- Install [jdk](https://docs.aws.amazon.com/corretto/latest/corretto-17-ug/downloads-list.html) and [cmake](https://cmake.org/download/).
+- Set the **JAVA_HOME** system environment variable, and add **cmake** to your **PATH**.
+For Windows users, you should install [mingw-w64](https://github.com/skeeto/w64devkit) and add **bin** directory to **PATH**.
+- For Windows
+```bash
</code_context>
<issue_to_address>
**suggestion (typo):** Tighten the grammar around adding the bin directory to PATH.
Consider: “For Windows users, install [mingw-w64](...) and add the **bin** directory to your **PATH**.” This adds the missing article and clarifies that it’s the user’s PATH.
```suggestion
For Windows users, install [mingw-w64](https://github.com/skeeto/w64devkit) and add the **bin** directory to your **PATH**.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
build-jni.sh
Outdated
Comment on lines
+1
to
+3
| #!/bin/bash | ||
| set -e | ||
| cd wrapper-java/src/main |
There was a problem hiding this comment.
suggestion: 使用相对路径的 cd 会导致脚本在仓库根目录之外运行时变得脆弱
这依赖当前工作目录是仓库根目录，如果在其他位置调用就会出错。建议从脚本所在位置推导路径，比如 cd "$(dirname "$0")/wrapper-java/src/main"，这样无论从哪里调用都能正常工作。
Suggested change
| #!/bin/bash | |
| set -e | |
| cd wrapper-java/src/main | |
| #!/bin/bash | |
| set -e | |
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| cd "$SCRIPT_DIR/wrapper-java/src/main" |
Original comment in English
suggestion: Using a relative cd makes the script fragile when run from outside the repo root
This relies on the current working directory being the repo root, so it breaks when invoked from elsewhere. Prefer deriving the path from the script location, e.g. cd "$(dirname "$0")/wrapper-java/src/main", so it works regardless of where it’s called from.
Suggested change
| #!/bin/bash | |
| set -e | |
| cd wrapper-java/src/main | |
| #!/bin/bash | |
| set -e | |
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| cd "$SCRIPT_DIR/wrapper-java/src/main" |
build-jni.sh
Outdated
| else | ||
| DYLIB_EXT="so" # Linux | ||
| fi | ||
| cp build/libquickjs-java-wrapper.$DYLIB_EXT ../../.. |
There was a problem hiding this comment.
suggestion: cp 中未加引号的路径在包含空格时会出问题
如果仓库所在路径中有空格（例如位于 Program Files 下），这个 cp 会失败。请同时给源路径和目标路径加上引号，例如 cp "build/libquickjs-java-wrapper.$DYLIB_EXT" "../../.."，以提高在此类路径下的健壮性。
Suggested change
| cp build/libquickjs-java-wrapper.$DYLIB_EXT ../../.. | |
| cp "build/libquickjs-java-wrapper.$DYLIB_EXT" "../../.." |
Original comment in English
suggestion: Unquoted paths in cp can break when the path contains spaces
If the repo is in a path with spaces (e.g. under Program Files), this cp will fail. Please quote both source and destination, e.g. cp "build/libquickjs-java-wrapper.$DYLIB_EXT" "../../..", to make it robust to such paths.
Suggested change
| cp build/libquickjs-java-wrapper.$DYLIB_EXT ../../.. | |
| cp "build/libquickjs-java-wrapper.$DYLIB_EXT" "../../.." |
| @@ -0,0 +1,11 @@ | |||
| @echo off | |||
| cd wrapper-java\src\main | |||
There was a problem hiding this comment.
suggestion: 批处理脚本中的相对 cd 和未加引号路径在某些环境下会导致失败
这些命令假定脚本从仓库根目录运行，并且目录路径中没有空格。为了提高健壮性，建议基于脚本所在路径构造路径（例如 cd /d "%~dp0wrapper-java\src\main"），并为所有路径参数加上引号。
Original comment in English
suggestion: Relative cd and unquoted paths in the batch script can cause failures in some environments
These commands assume the script is run from the repo root and that no directories contain spaces. To make it more robust, base paths on the script location (e.g. cd /d "%~dp0wrapper-java\src\main") and quote all path arguments.
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
1 participant
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.
Summary by Sourcery
添加跨平台构建脚本和配置更新，用于构建 JNI 共享库，包括对 Windows 的支持以及构建流程文档。
新功能：
改进：
Original summary in English
Summary by Sourcery
Add cross-platform build scripts and configuration updates for building the JNI shared library, including Windows support and documentation for the build process.
New Features:
Enhancements: