Add freebsd pkg #539
Add freebsd pkg #539
Conversation
WalkthroughThe pull request introduces a new GitHub Actions workflow for automating the building of a FreeBSD package from a Java project. It adds a new job to the existing CI workflow that calls this workflow, enhancing the CI/CD pipeline. Additionally, it includes a package manifest, installation scripts, and service management scripts for the PeerBanHelper application, facilitating its deployment and management. Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (8)
pkg/pkg/post-install (1)
3-3: LGTM: Clear instructions for enabling auto-startThe instruction for enabling auto-start is clear and follows FreeBSD conventions. However, we could improve it slightly for better user understanding.
Consider adding a brief explanation about what this command does. Here's a suggested improvement:
-echo "To auto start peerbanhelper, please run: sysrc peerbanhelper_enable=YES" +echo "To enable automatic startup of peerbanhelper on system boot, please run: sysrc peerbanhelper_enable=YES"This change provides more context about what "auto start" means in this context.
pkg/pkg/MANIFEST (2)
17-20: Specify the exact BSD licenseWhile "BSD" is specified as the license, it would be more precise to indicate which BSD license is being used (e.g., "BSD-2-Clause" or "BSD-3-Clause"). This helps users understand the exact terms of the license.
Consider updating the license specification to be more specific:
"licenses": [ - "BSD" + "BSD-2-Clause" ],(Replace "BSD-2-Clause" with the actual BSD license you're using if it's different)
5-6: Consider security implications of automatic peer banningThe package is described as a tool to "auto ban peers on the bittorrent network". While this can be useful for network management, it's important to ensure that:
- The banning mechanism is implemented securely and cannot be exploited.
- The tool complies with relevant network policies and laws.
- Users are provided with clear documentation on how the auto-banning works and how to configure it.
Consider adding a note in the package description or README about responsible use and any necessary precautions.
pkg/pkg/work-dir/usr/local/etc/rc.d/peerbanhelper (1)
21-22: LGTM: Correct daemon usage with minor suggestionThe use of daemon to run the Java application is a good practice. The PID file creation and running as the specified user are correctly implemented.
Consider adding a check to ensure the PID file directory exists and is writable:
+piddir="/var/run" +if [ ! -d "$piddir" ]; then + mkdir -p "$piddir" + chmod 755 "$piddir" +fi command="/usr/sbin/daemon" -command_args="-f -p /var/run/${name}.pid -u ${peerbanhelper_user} ${peerbanhelper_command}" +command_args="-f -p ${piddir}/${name}.pid -u ${peerbanhelper_user} ${peerbanhelper_command}"This ensures the script works even if the /var/run directory doesn't exist or has incorrect permissions.
.github/workflows/build_pkg.yml (3)
6-7: Consider renaming the job for clarity.The job name "dpkg" might be misleading as this workflow is building a FreeBSD package, not a Debian package. Consider renaming it to something more descriptive like "build_freebsd_pkg" to accurately reflect its purpose.
jobs: - dpkg: + build_freebsd_pkg: runs-on: ubuntu-latest
25-31: LGTM: FreeBSD package build step looks good.The use of a FreeBSD VM for building the package ensures compatibility, and the package creation command includes all necessary files. Displaying package information is a good practice for verification.
Consider adding error handling to ensure the workflow fails if the package creation is unsuccessful. You can achieve this by adding
set -eat the beginning of the script:run: | + set -e pkg create -M pkg/pkg/MANIFEST -r pkg/pkg/work-dir -p pkg/pkg/pkg-plist -o pkg/ pkg info -F pkg/*.pkgThis addition will cause the workflow to fail if any command in the script exits with a non-zero status.
1-37: Overall, the workflow is well-structured with room for minor improvements.The workflow successfully achieves its purpose of building a FreeBSD package. Here's a summary of the main points for improvement:
- Rename the job from "dpkg" to a more descriptive name like "build_freebsd_pkg".
- Pin the Java info action to a specific version for better reproducibility.
- Fix the potential word splitting issue in the sed command.
- Add error handling to the FreeBSD package build step.
These changes will enhance the clarity, reproducibility, and robustness of the workflow.
🧰 Tools
🪛 actionlint
17-17: shellcheck reported issue in this script: SC2086:info:2:43: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/jvm-ci.yml (1)
30-32: Consider adding a comment to explain the purpose of theBuild_PKGjob.While the addition of the
Build_PKGjob is clear and well-structured, it would be helpful to add a brief comment explaining its purpose, especially if it's introducing a new package format or target platform (e.g., FreeBSD pkg).Consider adding a comment above the job definition, like this:
# Build FreeBSD pkg package Build_PKG: needs: CI uses: ./.github/workflows/build_pkg.ymlThis will provide context for other developers and make the workflow more self-documenting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- .github/workflows/build_pkg.yml (1 hunks)
- .github/workflows/jvm-ci.yml (1 hunks)
- pkg/pkg/MANIFEST (1 hunks)
- pkg/pkg/pkg-plist (1 hunks)
- pkg/pkg/post-install (1 hunks)
- pkg/pkg/work-dir/usr/local/etc/rc.d/peerbanhelper (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/build_pkg.yml
17-17: shellcheck reported issue in this script: SC2086:info:2:43: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (11)
pkg/pkg/post-install (2)
1-1: LGTM: Correct shebang for shell scriptThe shebang
#!/bin/shis correct and ensures compatibility across different Unix-like systems.
1-3: Overall: Good post-install script with minor improvements neededThis post-install script follows FreeBSD conventions and provides essential information to the user. It correctly informs about the config file location and how to enable auto-start using the appropriate
sysrccommand.A few minor improvements have been suggested:
- Fixing a typo in the first echo statement.
- Enhancing clarity in the auto-start instruction.
Once these small changes are made, the script will be ready for use.
pkg/pkg/MANIFEST (3)
9-14: LGTM: Dependencies are well-definedThe dependency on openjdk21 is clearly specified with its origin and version. This is good practice and helps ensure reproducibility.
15-16: LGTM: Category and prefix are appropriateThe category "net" is suitable for a BitTorrent-related tool, and the prefix "/usr/local" follows FreeBSD conventions.
1-24: LGTM: Well-structured manifestThe overall structure and formatting of the manifest file are correct. The JSON is valid, and the indentation is consistent.
pkg/pkg/work-dir/usr/local/etc/rc.d/peerbanhelper (3)
1-8: LGTM: Correct script header and rc.subr inclusionThe script header follows FreeBSD conventions for rc scripts. The shebang, PROVIDE, REQUIRE, and KEYWORD directives are correctly set. Sourcing rc.subr is properly done, which is necessary for using the rc framework functions.
9-12: LGTM: Correct name and rcvar setupThe script name and rcvar are correctly set following FreeBSD conventions. The rc.conf is properly loaded using load_rc_config, which allows for user-defined configurations to be applied.
23-23: LGTM: Correct use of run_rc_commandThe use of run_rc_command is the correct way to handle start/stop/restart commands in FreeBSD rc scripts. This ensures that the script integrates properly with the FreeBSD rc system.
.github/workflows/build_pkg.yml (2)
1-4: LGTM: Workflow name and trigger are well-defined.The workflow name accurately describes its purpose, and using
workflow_callas the trigger allows for reusability, which is a good practice for maintaining modular and efficient CI/CD pipelines.
32-37: LGTM: Artifact upload step is well-configured.The artifact upload step is correctly configured to capture all built .pkg files and make them available for subsequent steps or workflows. This is a good practice for maintaining build artifacts.
.github/workflows/jvm-ci.yml (1)
30-32: LGTM! The newBuild_PKGjob is well-integrated.The addition of the
Build_PKGjob is consistent with the existing workflow structure. It follows the same pattern as other build jobs, depending on theCIjob and using a separate workflow file.To ensure the referenced workflow file exists, run the following script:
This script will verify the existence of the
build_pkg.ymlfile and display its contents for additional context.✅ Verification successful
Verified: The
Build_PKGjob is correctly added and integrates seamlessly into the workflow.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the build_pkg.yml workflow file # Test: Check if the build_pkg.yml file exists if [ -f ".github/workflows/build_pkg.yml" ]; then echo "The build_pkg.yml file exists." else echo "Error: The build_pkg.yml file is missing." exit 1 fi # Additional context: Display the content of the build_pkg.yml file echo "Content of build_pkg.yml:" cat ".github/workflows/build_pkg.yml"Length of output: 1413
| /usr/local/lib/peerbanhelper/PeerBanHelper.jar | ||
| /usr/local/etc/rc.d/peerbanhelper |
There was a problem hiding this comment.
💡 Codebase verification
Issues Detected: Missing Files and Dependencies
/usr/local/lib/peerbanhelper/PeerBanHelper.jarnot found./usr/local/etc/rc.d/peerbanhelperrc script not found.- Configuration file
/usr/local/etc/peerbanhelper.confnot found. - Java is not installed on the system.
These issues indicate that the pkg-plist references files that are missing and necessary dependencies are not specified.
🔗 Analysis chain
LGTM! Consider adding configuration and specifying dependencies.
The pkg-plist file correctly lists the JAR file and the rc script for the PeerBanHelper. However, there are a few suggestions to improve the package:
- Consider adding a configuration file (e.g.,
/usr/local/etc/peerbanhelper.conf) if the application requires one. - Ensure that any required dependencies (such as Java runtime) are specified in the package metadata.
- Consider specifying file permissions in the pkg-plist, especially for the rc script.
You might want to add the following lines to the pkg-plist:
@sample /usr/local/etc/peerbanhelper.conf.sample
@(root,wheel,555) /usr/local/etc/rc.d/peerbanhelper
This will add a sample configuration file and set appropriate permissions for the rc script.
To ensure the package is set up correctly, you can run the following script:
This script will help verify the presence and permissions of the package files, as well as check for Java installation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the PeerBanHelper package setup
# Check if the JAR file exists
if [ -f /usr/local/lib/peerbanhelper/PeerBanHelper.jar ]; then
echo "PeerBanHelper.jar found"
else
echo "PeerBanHelper.jar not found"
fi
# Check if the rc script exists and has correct permissions
if [ -f /usr/local/etc/rc.d/peerbanhelper ]; then
echo "peerbanhelper rc script found"
ls -l /usr/local/etc/rc.d/peerbanhelper
else
echo "peerbanhelper rc script not found"
fi
# Check for a configuration file
if [ -f /usr/local/etc/peerbanhelper.conf ]; then
echo "Configuration file found"
else
echo "Configuration file not found"
fi
# Check if Java is installed
if command -v java >/dev/null 2>&1; then
echo "Java is installed"
else
echo "Java is not installed"
fi
Length of output: 433
| @@ -0,0 +1,3 @@ | |||
| #!/bin/sh | |||
| echo "Thansk for using peerbanhelper. Your config file will be placed at /usr/local/etc/peerbanhelper" | |||
There was a problem hiding this comment.
Fix typo in the echo statement
There's a typo in the word "Thanks" at the beginning of the echo statement.
Please apply the following correction:
-echo "Thansk for using peerbanhelper. Your config file will be placed at /usr/local/etc/peerbanhelper"
+echo "Thanks for using peerbanhelper. Your config file will be placed at /usr/local/etc/peerbanhelper"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "Thansk for using peerbanhelper. Your config file will be placed at /usr/local/etc/peerbanhelper" | |
| echo "Thanks for using peerbanhelper. Your config file will be placed at /usr/local/etc/peerbanhelper" |
| { | ||
| "name": "peerbanhelper", | ||
| "version": "<version>", | ||
| "origin": "net/peerbanhelper", | ||
| "comment": "Tool to auto ban peers on the bittorrent network", | ||
| "desc": "PeerBanHelper is a tool to auto ban peers on the bittorrent network", | ||
| "maintainer": "[email protected]", | ||
| "www": "https://github.com/PBH-BTN/PeerBanHelper", |
There was a problem hiding this comment.
Replace version placeholder and verify maintainer email
- The version field contains a placeholder "". Please replace this with the actual version number of the package.
- The maintainer's email domain (111.com) seems unusual. Please verify if this is the correct email address or if it needs to be updated.
Apply this diff to fix the version placeholder (replace X.Y.Z with the actual version number):
- "version": "<version>",
+ "version": "X.Y.Z",Committable suggestion was skipped due to low confidence.
| "scripts": { | ||
| "post-install": "post-install" | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Post-install script requires enhancements
The post-install script exists but currently only echoes informational messages. It does not perform any post-installation tasks. Additionally, there's a typo in the first echo statement ("Thansk" should be "Thanks"). Please update the script to include necessary setup actions and correct the typo to ensure it follows best practices for FreeBSD package post-install scripts.
🔗 Analysis chain
Verify post-install script
A post-install script is specified, but its contents are not visible in this manifest. Please ensure that:
- The "post-install" script file exists in the appropriate location.
- The script performs the necessary post-installation tasks.
- The script follows best practices for FreeBSD package post-install scripts.
Run the following script to check for the existence of the post-install script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the existence of the post-install script
# Test: Search for the post-install script. Expect: File exists and is not empty.
fd -t f 'post-install' | xargs -I {} sh -c 'if [ -s "{}" ]; then echo "Found non-empty post-install script: {}"; else echo "Error: post-install script not found or empty: {}"; fi'
Length of output: 239
Script:
#!/bin/bash
# Description: Display the contents of the post-install script
# Test: Output the contents of pkg/pkg/post-install
cat pkg/pkg/post-install
Length of output: 218
| : ${peerbanhelper_enable:=NO} | ||
| : ${peerbanhelper_user:="root"} | ||
| : ${peerbanhelper_dir:="/usr/local/etc/peerbanhelper"} | ||
| : ${peerbanhelper_classpath:="/usr/local/lib/peerbanhelper/PeerBanHelper.jar"} | ||
| : ${peerbanhelper_jvm_flags:="-Dpbh.datadir=${peerbanhelper_dir} -Xmx386M -XX:+UseG1GC -XX:+UseStringDeduplication -XX:+ShrinkHeapInSteps"} | ||
| : ${peerbanhelper_command:="/usr/local/bin/java ${peerbanhelper_jvm_flags} -jar ${peerbanhelper_classpath}"} |
There was a problem hiding this comment.
Consider security improvements and flexible configuration
While the default configuration is generally good, there are a few points to consider:
- Running as root by default is a security risk. Consider using a dedicated non-privileged user.
- The memory limit of 386M might be too low for some systems. Consider making this more easily configurable.
- The data directory is set to /usr/local/etc, which might not be the best location for variable data.
Suggested improvements:
- Change the default user:
-: ${peerbanhelper_user:="root"}
+: ${peerbanhelper_user:="peerbanhelper"}- Make the memory limit more easily configurable:
-: ${peerbanhelper_jvm_flags:="-Dpbh.datadir=${peerbanhelper_dir} -Xmx386M -XX:+UseG1GC -XX:+UseStringDeduplication -XX:+ShrinkHeapInSteps"}
+: ${peerbanhelper_memory:="386M"}
+: ${peerbanhelper_jvm_flags:="-Dpbh.datadir=${peerbanhelper_dir} -Xmx${peerbanhelper_memory} -XX:+UseG1GC -XX:+UseStringDeduplication -XX:+ShrinkHeapInSteps"}- Consider changing the default data directory:
-: ${peerbanhelper_dir:="/usr/local/etc/peerbanhelper"}
+: ${peerbanhelper_dir:="/var/db/peerbanhelper"}These changes will improve security and flexibility of the configuration.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| : ${peerbanhelper_enable:=NO} | |
| : ${peerbanhelper_user:="root"} | |
| : ${peerbanhelper_dir:="/usr/local/etc/peerbanhelper"} | |
| : ${peerbanhelper_classpath:="/usr/local/lib/peerbanhelper/PeerBanHelper.jar"} | |
| : ${peerbanhelper_jvm_flags:="-Dpbh.datadir=${peerbanhelper_dir} -Xmx386M -XX:+UseG1GC -XX:+UseStringDeduplication -XX:+ShrinkHeapInSteps"} | |
| : ${peerbanhelper_command:="/usr/local/bin/java ${peerbanhelper_jvm_flags} -jar ${peerbanhelper_classpath}"} | |
| : ${peerbanhelper_enable:=NO} | |
| : ${peerbanhelper_user:="peerbanhelper"} | |
| : ${peerbanhelper_dir:="/var/db/peerbanhelper"} | |
| : ${peerbanhelper_classpath:="/usr/local/lib/peerbanhelper/PeerBanHelper.jar"} | |
| : ${peerbanhelper_memory:="386M"} | |
| : ${peerbanhelper_jvm_flags:="-Dpbh.datadir=${peerbanhelper_dir} -Xmx${peerbanhelper_memory} -XX:+UseG1GC -XX:+UseStringDeduplication -XX:+ShrinkHeapInSteps"} | |
| : ${peerbanhelper_command:="/usr/local/bin/java ${peerbanhelper_jvm_flags} -jar ${peerbanhelper_classpath}"} |
| - uses: actions/checkout@v4 | ||
| - id: java_info | ||
| uses: YunaBraska/java-info-action@main | ||
| - uses: actions/download-artifact@v4 | ||
| with: | ||
| name: maven-dist | ||
| path: target/ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin the Java info action to a specific version.
While using the latest version of actions/checkout is good practice, it's recommended to pin the YunaBraska/java-info-action to a specific version to ensure reproducibility and prevent unexpected changes in your workflow.
Consider updating the Java info action step as follows:
- id: java_info
- uses: YunaBraska/java-info-action@main
+ uses: YunaBraska/[email protected]Replace v1.12.0 with the latest stable version available at the time of implementation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@v4 | |
| - id: java_info | |
| uses: YunaBraska/java-info-action@main | |
| - uses: actions/download-artifact@v4 | |
| with: | |
| name: maven-dist | |
| path: target/ | |
| - uses: actions/checkout@v4 | |
| - id: java_info | |
| uses: YunaBraska/java-info-action@v1.12.0 | |
| - uses: actions/download-artifact@v4 | |
| with: | |
| name: maven-dist | |
| path: target/ |
.github/workflows/build_pkg.yml
Outdated
| run: | | ||
| calculatedSha=$(git rev-parse --short ${{ github.sha }}) | ||
| echo "COMMIT_SHORT_SHA=$calculatedSha" >> $GITHUB_ENV | ||
| sed -i "s/<version>/${{ env.PBH_VERSION }}/g" pkg/pkg/MANIFEST | ||
| mkdir -p pkg/work-dir/usr/local/lib/peerbanhelper | ||
| cp target/PeerBanHelper.jar pkg/work-dir/usr/local/lib/peerbanhelper/ | ||
| env: | ||
| PBH_VERSION: ${{ steps.java_info.outputs.project_version }} |
There was a problem hiding this comment.
Fix potential word splitting issue in sed command.
The sed command on line 20 might be susceptible to word splitting if the version contains spaces or special characters. To prevent this, we should quote the variable.
Apply this diff to fix the potential word splitting issue:
- sed -i "s/<version>/${{ env.PBH_VERSION }}/g" pkg/pkg/MANIFEST
+ sed -i "s/<version>/${PBH_VERSION}/g" pkg/pkg/MANIFESTThis change ensures that the version is treated as a single string, regardless of any spaces or special characters it might contain.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: | | |
| calculatedSha=$(git rev-parse --short ${{ github.sha }}) | |
| echo "COMMIT_SHORT_SHA=$calculatedSha" >> $GITHUB_ENV | |
| sed -i "s/<version>/${{ env.PBH_VERSION }}/g" pkg/pkg/MANIFEST | |
| mkdir -p pkg/work-dir/usr/local/lib/peerbanhelper | |
| cp target/PeerBanHelper.jar pkg/work-dir/usr/local/lib/peerbanhelper/ | |
| env: | |
| PBH_VERSION: ${{ steps.java_info.outputs.project_version }} | |
| run: | | |
| calculatedSha=$(git rev-parse --short ${{ github.sha }}) | |
| echo "COMMIT_SHORT_SHA=$calculatedSha" >> $GITHUB_ENV | |
| sed -i "s/<version>/${PBH_VERSION}/g" pkg/pkg/MANIFEST | |
| mkdir -p pkg/work-dir/usr/local/lib/peerbanhelper | |
| cp target/PeerBanHelper.jar pkg/work-dir/usr/local/lib/peerbanhelper/ | |
| env: | |
| PBH_VERSION: ${{ steps.java_info.outputs.project_version }} |
🧰 Tools
🪛 actionlint
17-17: shellcheck reported issue in this script: SC2086:info:2:43: Double quote to prevent globbing and word splitting
(shellcheck)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.github/workflows/build_pkg.yml (3)
1-7: LGTM! Consider pinning the Ubuntu version for consistency.The workflow setup looks good. The name is descriptive, and using
workflow_callallows for reusability. However, you might want to consider pinning the Ubuntu version to a specific release for consistency across runs.You could update the
runs-onfield like this:- runs-on: ubuntu-latest + runs-on: ubuntu-22.04Replace
22.04with the desired Ubuntu version.
23-33: LGTM! Consider adding error checking to the build process.The FreeBSD package build step is well-structured and uses appropriate actions. The directory setup, file copying, and package creation seem correct.
To improve robustness, consider adding error checking to the build process. For example:
run: | cd PeerBanHelper - ls + ls || { echo "Failed to list directory contents"; exit 1; } mkdir -p pkg/pkg/work-dir/usr/local/lib/peerbanhelper - cp target/PeerBanHelper.jar pkg/pkg/work-dir/usr/local/lib/peerbanhelper/ + cp target/PeerBanHelper.jar pkg/pkg/work-dir/usr/local/lib/peerbanhelper/ || { echo "Failed to copy JAR file"; exit 1; } - pkg create -M pkg/pkg/MANIFEST -r /pkg/pkg/work-dir -p pkg/pkg/pkg-plist -o pkg/ + pkg create -M pkg/pkg/MANIFEST -r /pkg/pkg/work-dir -p pkg/pkg/pkg-plist -o pkg/ || { echo "Failed to create package"; exit 1; } pkg info -F pkg/*.pkgThis will help catch and report any failures during the build process.
1-39: Overall, the workflow is well-structured but has room for improvement.The FreeBSD package build workflow is generally well-designed and achieves its purpose. However, there are a few areas where it can be improved:
- Consider pinning the Ubuntu version for consistency.
- Pin the Java info action to a specific version.
- Fix the potential word splitting issue in the sed command.
- Add error checking to the FreeBSD package build process.
Implementing these suggestions will enhance the workflow's reliability and consistency.
🧰 Tools
🪛 actionlint
17-17: shellcheck reported issue in this script: SC2086:info:2:43: Double quote to prevent globbing and word splitting
(shellcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/build_pkg.yml (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/build_pkg.yml
17-17: shellcheck reported issue in this script: SC2086:info:2:43: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (4)
.github/workflows/build_pkg.yml (4)
9-11: Pin the Java info action to a specific version.While using the latest version of
actions/checkoutis good practice, it's recommended to pin theYunaBraska/java-info-actionto a specific version to ensure reproducibility and prevent unexpected changes in your workflow.As suggested in a previous review, consider updating the Java info action step as follows:
- id: java_info - uses: YunaBraska/java-info-action@main + uses: YunaBraska/[email protected]Replace
v1.12.0with the latest stable version available at the time of implementation.
12-15: LGTM! Artifact download step is well-configured.The artifact download step is correctly set up, using the latest version of the action and specifying both the artifact name and download path.
16-22: Fix potential word splitting issue in sed command.The version replacement step looks good overall, but there's a potential issue with the sed command that could lead to word splitting if the version contains spaces or special characters.
As suggested in a previous review, apply this diff to fix the potential word splitting issue:
- sed -i "s/<version>/${{ env.PBH_VERSION }}/g" pkg/pkg/MANIFEST + sed -i "s/<version>/${PBH_VERSION}/g" pkg/pkg/MANIFESTThis change ensures that the version is treated as a single string, regardless of any spaces or special characters it might contain.
🧰 Tools
🪛 actionlint
17-17: shellcheck reported issue in this script: SC2086:info:2:43: Double quote to prevent globbing and word splitting
(shellcheck)
34-39: LGTM! Artifact upload step is well-configured.The artifact upload step is correctly set up, using the latest version of the action and properly specifying the artifact name and path to the built package files.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/build_pkg.yml (1)
23-31: LGTM: FreeBSD package build step is well-structured.The use of
vmactions/freebsd-vmand the subsequent package creation steps are correctly implemented. However, consider adding a comment explaining the purpose of each command for better readability and maintainability.Here's a suggested improvement:
- name: Build FreeBSD pkg uses: vmactions/freebsd-vm@v1 with: usesh: true run: | + # Create directory structure mkdir -p pkg/pkg/work-dir/usr/local/lib/peerbanhelper + # Copy JAR file to the appropriate location cp target/PeerBanHelper.jar pkg/pkg/work-dir/usr/local/lib/peerbanhelper/ + # Create the FreeBSD package pkg create -M pkg/pkg/MANIFEST -r /pkg/pkg/work-dir -p pkg/pkg/pkg-plist -o pkg/ + # List the contents of the pkg directory ls -l pkg
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/build_pkg.yml (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/build_pkg.yml
17-17: shellcheck reported issue in this script: SC2086:info:2:43: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (6)
.github/workflows/build_pkg.yml (6)
1-7: LGTM: Workflow setup looks good.The workflow is well-structured with a
workflow_calltrigger, allowing for reusability. Running the job onubuntu-latestensures compatibility with the most recent Ubuntu version on GitHub Actions.
9-11: Pin the Java info action to a specific version.While using the latest version of
actions/checkoutis good practice, it's recommended to pin theYunaBraska/java-info-actionto a specific version to ensure reproducibility and prevent unexpected changes in your workflow.Consider updating the Java info action step as follows:
- id: java_info - uses: YunaBraska/java-info-action@main + uses: YunaBraska/[email protected]Replace
v1.12.0with the latest stable version available at the time of implementation.
12-15: LGTM: Artifact download step is well-configured.The use of the latest version of
actions/download-artifactand the appropriate target directory for Maven projects demonstrates good practices in workflow design.
17-22: Fix potential word splitting issue in sed command.The
sedcommand on line 20 might be susceptible to word splitting if the version contains spaces or special characters. To prevent this, we should quote the variable.Apply this diff to fix the potential word splitting issue:
- sed -i "s/<version>/${{ env.PBH_VERSION }}/g" pkg/pkg/MANIFEST + sed -i "s/<version>/${PBH_VERSION}/g" pkg/pkg/MANIFESTThis change ensures that the version is treated as a single string, regardless of any spaces or special characters it might contain.
🧰 Tools
🪛 actionlint
17-17: shellcheck reported issue in this script: SC2086:info:2:43: Double quote to prevent globbing and word splitting
(shellcheck)
32-37: LGTM: Artifact upload step is correctly implemented.The use of the latest version of
actions/upload-artifactand the correct specification of the artifact name and path demonstrate good practices in workflow design.
1-37: LGTM: Overall workflow structure is well-designed and complete.The workflow efficiently covers all necessary steps for building a FreeBSD package, from code checkout to artifact upload. It demonstrates good practices in GitHub Actions workflow design, including modularity (through
workflow_call), proper use of actions, and clear step organization.🧰 Tools
🪛 actionlint
17-17: shellcheck reported issue in this script: SC2086:info:2:43: Double quote to prevent globbing and word splitting
(shellcheck)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/build_pkg.yml (1)
1-8: LGTM! Consider specifying Ubuntu version for consistency.The workflow setup looks good. Using
workflow_callallows for reusability, which is excellent. However, consider specifying a particular Ubuntu version (e.g.,ubuntu-22.04) instead ofubuntu-latestto ensure consistent behavior across different runs.- runs-on: ubuntu-latest + runs-on: ubuntu-22.04
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/build_pkg.yml (1 hunks)
- pkg/pkg/post-install (1 hunks)
- pkg/pkg/work-dir/usr/local/etc/rc.d/peerbanhelper (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/pkg/post-install
- pkg/pkg/work-dir/usr/local/etc/rc.d/peerbanhelper
🧰 Additional context used
🪛 actionlint
.github/workflows/build_pkg.yml
17-17: shellcheck reported issue in this script: SC2086:info:2:43: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (5)
.github/workflows/build_pkg.yml (5)
9-15: Pin Java info action to a specific version.The checkout and artifact download steps look good. However, as mentioned in a previous review, it's recommended to pin the
YunaBraska/java-info-actionto a specific version for better reproducibility.Apply this change:
- id: java_info - uses: YunaBraska/java-info-action@main + uses: YunaBraska/[email protected]Replace
v1.12.0with the latest stable version available at the time of implementation.
16-22: Fix potential word splitting issue in sed command.The version replacement step looks good overall, but there's a potential word splitting issue in the
sedcommand, as mentioned in a previous review.Apply this change to fix the issue:
- sed -i "s/<version>/${{ env.PBH_VERSION }}/g" pkg/pkg/MANIFEST + sed -i "s/<version>/${PBH_VERSION}/g" pkg/pkg/MANIFESTThis ensures that the version is treated as a single string, regardless of any spaces or special characters it might contain.
🧰 Tools
🪛 actionlint
17-17: shellcheck reported issue in this script: SC2086:info:2:43: Double quote to prevent globbing and word splitting
(shellcheck)
23-31: LGTM! FreeBSD package build step looks good.The FreeBSD package build step is well-structured and uses appropriate actions and commands. The directory structure creation, JAR file copying, and package creation all appear to be correctly implemented.
32-37: LGTM! Artifact upload step is well-implemented.The artifact upload step is correctly implemented, using the latest version of the
upload-artifactaction. The artifact name and path are appropriately set to capture the built FreeBSD package.
1-37: Overall, the FreeBSD package build workflow looks good with minor improvements suggested.The workflow is well-structured and appropriate for building a FreeBSD package. It includes all necessary steps from code checkout to artifact upload. The suggested improvements are:
- Consider specifying a particular Ubuntu version for consistency.
- Pin the Java info action to a specific version.
- Fix the potential word splitting issue in the
sedcommand.These changes will enhance the reproducibility and reliability of the workflow.
🧰 Tools
🪛 actionlint
17-17: shellcheck reported issue in this script: SC2086:info:2:43: Double quote to prevent globbing and word splitting
(shellcheck)
|
@Ghost-chu need to change the collect script |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.github/workflows/build_pkg.yml (2)
23-31: LGTM: FreeBSD package build step is well-configured.The use of a FreeBSD VM for building the package is appropriate, and the steps for package creation are correct. For consistency with the previous step, consider using
${{ env.PBH_VERSION }}instead of${{ env.PBH_VERSION }}in the package info command.Apply this minor change for consistency:
- pkg info -F pkg/peerbanhelper-${{ env.PBH_VERSION }}.pkg + pkg info -F pkg/peerbanhelper-${PBH_VERSION}.pkg
1-37: Overall, the workflow is well-structured and comprehensive.The workflow effectively covers all necessary steps for building a FreeBSD package, including proper error handling and artifact management. It follows good practices in GitHub Actions workflows.
To further improve the workflow:
- Consider adding a step to validate the FreeBSD package after creation, such as attempting to install it in the VM or running additional checks.
- You might want to add a conditional step to handle cases where the package build fails, perhaps by uploading logs or sending a notification.
- If this package is intended for release, consider adding a step to create a GitHub release with the built package.
🧰 Tools
🪛 actionlint
17-17: shellcheck reported issue in this script: SC2086:info:2:43: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/jvm-release.yml (1)
70-73: LGTM! Consider adding a comment for clarity.The addition of this step to download the 'pkg-dist' artifact is appropriate and aligns with the PR objective of adding a FreeBSD package. This ensures that the FreeBSD package is available for the subsequent upload-assets action.
For consistency and clarity, consider adding a comment above this step, similar to the comments present for other artifact downloads. For example:
# Download FreeBSD package artifact - uses: actions/download-artifact@v4 with: name: pkg-dist path: target/media
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/build_pkg.yml (1 hunks)
- .github/workflows/jvm-release.yml (1 hunks)
- install4j/project.install4j (0 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- install4j/project.install4j
🧰 Additional context used
🪛 actionlint
.github/workflows/build_pkg.yml
17-17: shellcheck reported issue in this script: SC2086:info:2:43: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (7)
.github/workflows/build_pkg.yml (5)
1-7: LGTM: Workflow setup is correct.The workflow is properly set up as a reusable workflow triggered by
workflow_call. The job configuration using the latest Ubuntu is appropriate for this package building task.
9-11: Pin the Java info action to a specific version.While using the latest version of
actions/checkoutis good practice, it's recommended to pin theYunaBraska/java-info-actionto a specific version to ensure reproducibility and prevent unexpected changes in your workflow.
12-15: LGTM: Artifact download step is correctly configured.The step to download the previously built Maven distribution artifact is properly set up. It uses the latest version of the
actions/download-artifactaction and specifies appropriate naming and path for a Maven project.
16-22: Fix potential word splitting issue in sed command.The
sedcommand on line 20 might be susceptible to word splitting if the version contains spaces or special characters. To prevent this, we should quote the variable.🧰 Tools
🪛 actionlint
17-17: shellcheck reported issue in this script: SC2086:info:2:43: Double quote to prevent globbing and word splitting
(shellcheck)
32-37: LGTM: Artifact upload step is correctly configured.The step to upload the built package artifact is properly set up. It uses the latest version of the
actions/upload-artifactaction and specifies appropriate naming and path for the FreeBSD package files..github/workflows/jvm-release.yml (2)
Line range hint
1-78: Overall changes look good, consider additional steps for full FreeBSD support.The modifications to include the FreeBSD package in the release process are appropriate and align with the PR objective. The changes are minimal and focused, which is good for maintaining the existing workflow.
To ensure full support for the FreeBSD package, consider the following next steps:
- Review and update the
Build_Executablejob (if necessary) to ensure it generates the FreeBSD package.- Check if any changes are needed in the
Build_Installersjob to support FreeBSD.- Update documentation or README files to mention the new FreeBSD package support.
- Consider adding a new job specifically for building the FreeBSD package, similar to the existing
Build_DEBjob.To ensure the FreeBSD package is being correctly generated and included, you can add a step in the
Upload_Artifactsjob to list the contents of thetarget/mediadirectory:- name: List artifacts run: ls -R target/mediaThis will help verify that the FreeBSD package is present before the upload step.
78-78:⚠️ Potential issueAdjust asset_paths to include all required files and fix syntax error.
The modification of the asset_paths parameter broadens the pattern for matching files to be uploaded, which is good for including the new FreeBSD package. However, there are a couple of issues to address:
- The new pattern might exclude .spk files if they don't match "peerbanhelper-*".
- There's a syntax error: the closing bracket ']' is missing at the end of the asset_paths array.
To resolve these issues, please modify the line as follows:
- asset_paths: '["target/PeerBanHelper.jar", "target/media/PeerBanHelper_*", "target/media/peerbanhelper_*", "target/media/peerbanhelper-*"' + asset_paths: '["target/PeerBanHelper.jar", "target/media/PeerBanHelper_*", "target/media/peerbanhelper_*", "target/media/peerbanhelper-*", "target/media/*.spk"]'This change ensures that:
- All previously included file patterns are maintained.
- The new FreeBSD package (likely matching "peerbanhelper-*") is included.
- The .spk files are still included.
- The syntax error is fixed by adding the closing bracket.
To ensure all necessary files are included, please run the following command in your local environment after building all artifacts:
If this command returns any results, consider adding those file patterns to the asset_paths as well.
|
@Gaojianli 我改完了，但它爆了 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/build_pkg.yml (1)
23-33: LGTM: FreeBSD package build step looks good.The FreeBSD package build step is well-structured and uses appropriate commands. However, consider adding error checking to ensure critical steps succeed.
Add error checking to critical steps:
run: | + set -e # Exit immediately if a command exits with a non-zero status mkdir -p pkg/pkg/work-dir/usr/local/lib/peerbanhelper cp target/PeerBanHelper.jar pkg/pkg/work-dir/usr/local/lib/peerbanhelper/ pkg create -M pkg/pkg/MANIFEST -r pkg/pkg/work-dir -p pkg/pkg/pkg-plist -o pkg/ pkg info -F pkg/peerbanhelper-${{ env.PBH_VERSION }}.pkgThis change ensures that the workflow fails immediately if any of these critical steps encounter an error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/build_pkg.yml (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/build_pkg.yml
17-17: shellcheck reported issue in this script: SC2086:info:2:43: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (5)
.github/workflows/build_pkg.yml (5)
1-7: LGTM: Workflow setup is appropriate.The workflow is correctly set up as a reusable workflow triggered by
workflow_call. The job configuration using the latest Ubuntu is suitable for this package building task.
9-11: Pin the Java info action to a specific version.While using the latest version of
actions/checkoutis good practice, it's recommended to pin theYunaBraska/java-info-actionto a specific version to ensure reproducibility and prevent unexpected changes in your workflow.
17-22: Fix potential word splitting issue in sed command.The
sedcommand on line 20 might be susceptible to word splitting if the version contains spaces or special characters. To prevent this, we should quote the variable.🧰 Tools
🪛 actionlint
17-17: shellcheck reported issue in this script: SC2086:info:2:43: Double quote to prevent globbing and word splitting
(shellcheck)
34-39: LGTM: Artifact upload step is correctly implemented.The artifact upload step uses the latest version of the
upload-artifactaction and correctly specifies the path for the built package files.
1-39: Overall, the FreeBSD package build workflow is well-implemented.The workflow covers all necessary steps for building a FreeBSD package, including code checkout, Java project info retrieval, package creation, and artifact handling. It uses appropriate actions and commands for each step.
A few minor improvements have been suggested:
- Pinning the Java info action to a specific version.
- Fixing a potential word splitting issue in the sed command.
- Adding error checking to critical steps in the FreeBSD VM.
Once these small issues are addressed, the workflow should be robust and ready for use.
🧰 Tools
🪛 actionlint
17-17: shellcheck reported issue in this script: SC2086:info:2:43: Double quote to prevent globbing and word splitting
(shellcheck)
Summary by CodeRabbit
New Features
peerbanhelper, for managing peer banning on the BitTorrent network.peerbanhelperservice.Documentation
peerbanhelper.Bug Fixes