Feature: Use argument files #70
Conversation
infeo
force-pushed
the
feature/argument-files
branch
from
00ba938 to
206a572
Compare
WalkthroughThe pull request introduces modifications across multiple workflow and script files aimed at enhancing the build process for a Java application. The primary change involves the transition from inline command arguments to the use of external argument files ( Additionally, new environment variables ( 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: 11
🧹 Outside diff range and nitpick comments (8)
build_mac.sh (2)
42-45: Add error handling for envsubst operationThe script should verify that the argument files are successfully processed before proceeding.
Add error checking:
envsubst < dist/jpackage.args > target/jpackage.args +if [ $? -ne 0 ] || [ ! -s ./target/jpackage.args ]; then + echo "Failed to process jpackage.args" + exit 1 +fi echo "Creating app binary with jpackage..." "$JAVA_HOME/bin/jpackage" '@./target/jpackage.args'
33-45: Document argument file format and environment variablesThe switch to argument files is a good architectural choice for maintainability. Consider adding documentation about:
- The expected format and supported variables in jlink.args and jpackage.args
- Required environment variables and their impact
- Example argument files for reference
This will help future maintainers understand and modify the build configuration.
🧰 Tools
🪛 Shellcheck (0.9.0-1)
[warning] 41-41: JP_APP_VERSION appears unused. Verify use (or export if used externally).
(SC2034)
build_win.ps1 (3)
3-4: Document version variables and improve organizationConsider adding comments explaining the purpose of different version variables and consolidate their declarations. The separation between
$appVersionand$jpAppVersion(declared on line 37) reduces readability.+# Version for the application $appVersion='0.1.0-local' +# Version specifically for jpackage (required due to Windows version format restrictions) +$jpAppVersion='99.9.9'
36-42: Improve placeholder replacement logic and documentationThe current implementation could be more maintainable and better documented.
-## powershell does not have envsubst +# PowerShell equivalent of Unix 'envsubst' command for environment variable substitution +$replacements = @{ + '${APP_VERSION}' = $appVersion + '${JP_APP_VERSION}' = $jpAppVersion + '${NATIVE_ACCESS_PACKAGE}' = 'org.cryptomator.jfuse.win' +} + +if (-not (Test-Path './dist/jpackage.args')) { + throw "jpackage argument file './dist/jpackage.args' not found" +} + Get-Content -Path './dist/jpackage.args' | ForEach-Object { - $_.Replace('${APP_VERSION}', $appVersion). - Replace('${JP_APP_VERSION}', $jpAppVersion). - Replace('${NATIVE_ACCESS_PACKAGE}', 'org.cryptomator.jfuse.win') + $line = $_ + $replacements.GetEnumerator() | ForEach-Object { + $line = $line.Replace($_.Key, $_.Value) + } + $line } | Out-File -FilePath './target/jpackage.args'
Line range hint
1-51: Consider cleanup of temporary filesThe script creates temporary files in the
targetdirectory but doesn't clean them up. Consider adding a cleanup step at the end of the script or using a temporary directory.+"$ErrorActionPreference = 'Stop'" + +# Cleanup function +function Cleanup { + if (Test-Path './target/jlink.args') { + Remove-Item './target/jlink.args' + } + if (Test-Path './target/jpackage.args') { + Remove-Item './target/jpackage.args' + } +} + +try { + # Existing script content here +} +finally { + Cleanup +}.github/workflows/build-win.yml (2)
74-76: Add validation for jpackage argument fileSimilar to the jlink command, consider adding validation for the jpackage argument file existence.
Consider adding error handling:
run: | + if [ ! -f "dist/jpackage.args" ]; then + echo "Error: dist/jpackage.args not found" + exit 1 + fi envsubst < dist/jpackage.args > target/jpackage.args "${JAVA_HOME}/bin/jpackage" '@./target/jpackage.args' --win-console
77-80: Document version environment variablesThe environment variables for versioning could benefit from inline documentation explaining their different purposes:
JP_APP_VERSION: Semantic version number (X.Y.Z)APP_VERSION: Full version stringConsider adding comments:
env: + # Semantic version number (X.Y.Z) for jpackage JP_APP_VERSION: ${{ needs.prepare.outputs.semVerNum }} + # Full version string including any pre-release identifiers APP_VERSION: ${{ needs.prepare.outputs.semVerStr }} NATIVE_ACCESS_PACKAGE: org.cryptomator.jfuse.win.github/workflows/build-mac.yml (1)
88-88: Consider moving NATIVE_ACCESS_PACKAGE to matrix configurationThe
NATIVE_ACCESS_PACKAGEis currently hardcoded for Mac. Consider moving this to the matrix configuration for better maintainability and consistency with other platform-specific configurations.Example modification:
strategy: matrix: include: - os: macos-latest architecture: arm64 artifact-name: cryptomator-cli-${{ needs.prepare.outputs.semVerStr }}-mac-arm64.zip + native-access-package: org.cryptomator.jfuse.mac - os: macos-13 architecture: x64 artifact-name: cryptomator-cli-${{ needs.prepare.outputs.semVerStr }}-mac-x64.zip + native-access-package: org.cryptomator.jfuse.macThen use it in the env section:
- NATIVE_ACCESS_PACKAGE: org.cryptomator.jfuse.mac + NATIVE_ACCESS_PACKAGE: ${{ matrix.native-access-package }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
dist/jlink.argsis excluded by!**/dist/**dist/jpackage.argsis excluded by!**/dist/**
📒 Files selected for processing (6)
.github/workflows/build-linux.yml(1 hunks).github/workflows/build-mac.yml(1 hunks).github/workflows/build-win.yml(1 hunks)build_linux.sh(3 hunks)build_mac.sh(2 hunks)build_win.ps1(3 hunks)
🧰 Additional context used
🪛 Shellcheck (0.9.0-1)
build_linux.sh
[warning] 6-6: APP_VERSION appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 56-56: JP_APP_VERSION appears unused. Verify use (or export if used externally).
(SC2034)
build_mac.sh
[warning] 5-5: APP_VERSION appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 41-41: JP_APP_VERSION appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (7)
build_linux.sh (1)
Line range hint 34-60: Verify the argument files exist and contain expected variables.
Let's ensure the argument files are present and properly formatted.
✅ Verification successful
The previous output showed the content of argument files but didn't clearly indicate their location. Let's verify both the source templates in dist/ and the generated files in target/, along with the environment variable definitions in the script.
Argument files are properly configured with required variables
The verification shows:
- Template files exist in
dist/with correct variable placeholders (${JP_APP_VERSION},${APP_VERSION},${NATIVE_ACCESS_PACKAGE}) - All required environment variables are properly defined in
build_linux.sh - The
target/directory error is expected as it's created during build time
The script correctly uses envsubst to populate these variables into the target files before executing jlink and jpackage commands.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if argument files exist and contain environment variables
echo "Checking argument files..."
# Look for the argument files
fd -t f "jlink.args|jpackage.args" -x cat {} \;
# Check for environment variable usage
echo -e "\nChecking for environment variables in argument files..."
rg -l '\$(?:APP_VERSION|JP_APP_VERSION)'
Length of output: 1328
Script:
#!/bin/bash
# Check the dist directory for argument files
echo "Checking dist directory for argument files..."
fd -t f "jlink.args|jpackage.args" dist/ -x cat {} \;
# Check target directory for generated argument files
echo -e "\nChecking target directory for generated argument files..."
fd -t f "jlink.args|jpackage.args" target/ -x cat {} \;
# Verify the environment variables used in the script
echo -e "\nVerifying environment variable definitions..."
rg "(?:JP_APP_VERSION|APP_VERSION|NATIVE_ACCESS_PACKAGE).*=" build_linux.sh
Length of output: 1909
🧰 Tools
🪛 Shellcheck (0.9.0-1)
[warning] 56-56: JP_APP_VERSION appears unused. Verify use (or export if used externally).
(SC2034)
.github/workflows/build-win.yml (2)
70-72: Verify argument file existence before processing
While the implementation correctly uses envsubst for processing the jlink arguments, it would be beneficial to add a validation step to ensure the argument file exists.
Let's verify the contents and existence of the jlink arguments file:
Consider adding error handling:
run: |
+ if [ ! -f "dist/jlink.args" ]; then
+ echo "Error: dist/jlink.args not found"
+ exit 1
+ fi
envsubst < dist/jlink.args > target/jlink.args
"${JAVA_HOME}/bin/jlink" '@./target/jlink.args'76-76: Consider moving --win-console to the argument file
For consistency with the argument file approach, consider moving the --win-console flag into the jpackage.args file.
Let's check if this flag is platform-specific in other workflow files:
.github/workflows/build-mac.yml (2)
82-88: 🛠️ Refactor suggestion
Add error handling and validate environment variables
The environment variables look good, but similar to jlink, we need better error handling.
Add error handling and environment variable validation:
run: |
+ # Validate environment variables
+ if [ -z "$JP_APP_VERSION" ] || [ -z "$APP_VERSION" ] || [ -z "$NATIVE_ACCESS_PACKAGE" ]; then
+ echo "Error: Required environment variables are not set"
+ exit 1
+ fi
+ mkdir -p target
+ if [ ! -f "dist/jpackage.args" ]; then
+ echo "Error: dist/jpackage.args not found"
+ exit 1
+ fi
envsubst < dist/jpackage.args > target/jpackage.args
+ if [ $? -ne 0 ]; then
+ echo "Error: Failed to process jpackage.args"
+ exit 1
+ fi
"${JAVA_HOME}/bin/jpackage" '@./target/jpackage.args'78-80: 🛠️ Refactor suggestion
Add error handling and directory verification
While the change to use argument files is good, consider adding some safeguards:
Add error handling and directory checks:
run: |
+ mkdir -p target
+ if [ ! -f "dist/jlink.args" ]; then
+ echo "Error: dist/jlink.args not found"
+ exit 1
+ fi
envsubst < dist/jlink.args > target/jlink.args
+ if [ $? -ne 0 ]; then
+ echo "Error: Failed to process jlink.args"
+ exit 1
+ fi
"${JAVA_HOME}/bin/jlink" '@./target/jlink.args'.github/workflows/build-linux.yml (2)
87-90: LGTM! Well-structured environment variables
The environment variable setup is well-organized:
- Clear separation between semantic version (APP_VERSION) and numeric version (JP_APP_VERSION)
- Proper scoping of variables to the specific step
- Good use of matrix variables for architecture-specific configuration
80-90: Verify the content and variables in argument files
Let's verify the argument files and their environment variable usage:
✅ Verification successful
Let's check the actual content of these files to verify their structure and variable usage.
Argument files are properly configured and environment variables are correctly used
The verification shows that both argument files exist and are properly configured:
jlink.argscontains valid JLink arguments with proper module configurationjpackage.argscorrectly uses all three environment variables defined in the workflow:${JP_APP_VERSION}for package versioning${APP_VERSION}for CLI version display${NATIVE_ACCESS_PACKAGE}for system-specific FUSE bindings
The workflow's environment variable declarations match the documented requirements in the argument files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for argument files and their content
echo "Checking for argument files..."
fd -t f ".*\.args$" dist/
echo -e "\nChecking environment variables used in argument files..."
if [ -f dist/jlink.args ]; then
echo "Variables in jlink.args:"
grep -o '\$[A-Z_]\+' dist/jlink.args || true
fi
if [ -f dist/jpackage.args ]; then
echo "Variables in jpackage.args:"
grep -o '\$[A-Z_]\+' dist/jpackage.args || true
fi
Length of output: 542
Script:
#!/bin/bash
echo "Content of dist/jlink.args:"
cat dist/jlink.args
echo -e "\nContent of dist/jpackage.args:"
cat dist/jpackage.args
Length of output: 1232
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
build_mac.sh (2)
41-42: Document version number and package name conventionsConsider adding comments to explain:
- Why
JP_APP_VERSIONis set to '99.9.9' (jpackage requirement)- The expected format for
NATIVE_ACCESS_PACKAGEApply this diff to add documentation:
+# jpackage requires version in Y.X.X format where Y >= 1 export JP_APP_VERSION='99.9.9' +# Mac-specific native access package for FUSE integration export NATIVE_ACCESS_PACKAGE="org.cryptomator.jfuse.mac"
42-44: Validate NATIVE_ACCESS_PACKAGE formatConsider adding validation for the package name format to catch configuration errors early.
Add this validation before envsubst:
export NATIVE_ACCESS_PACKAGE="org.cryptomator.jfuse.mac" +if [[ ! "$NATIVE_ACCESS_PACKAGE" =~ ^[a-z]+(\.[a-z]+)*$ ]]; then + echo "Invalid package name format in NATIVE_ACCESS_PACKAGE" + exit 1 +fi envsubst < dist/jpackage.args > target/jpackage.argsbuild_linux.sh (1)
34-35: Consider creating a shared function for argument file processing.Both jlink and jpackage use similar argument file processing logic. Consider creating a shared function to handle argument file processing, which would:
- Standardize error handling
- Ensure consistent path handling
- Reduce code duplication
- Make the script more maintainable
Example implementation:
process_args_file() { local input_file="$1" local output_file="$2" local tool_name="$3" SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" INPUT_PATH="${SCRIPT_DIR}/${input_file}" OUTPUT_PATH="${SCRIPT_DIR}/${output_file}" if [ ! -f "$INPUT_PATH" ]; then echo "Error: ${tool_name} arguments file not found at $INPUT_PATH" exit 1 } envsubst < "$INPUT_PATH" > "$OUTPUT_PATH" echo "@$OUTPUT_PATH" }Also applies to: 57-60
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
build_linux.sh(3 hunks)build_mac.sh(2 hunks)
🔇 Additional comments (4)
build_mac.sh (1)
33-34: Create target directory before envsubst
The script assumes the target directory exists when writing the processed args file.
build_linux.sh (3)
34-35: Previous review comment about error handling and absolute paths is still applicable.
The current implementation using relative paths could fail if the script is run from the wrong directory or if required directories don't exist.
42-42: LGTM! Good architecture detection and error handling.
The export and architecture-specific package selection is well implemented with appropriate warning messages for unsupported architectures.
57-60: Previous review comment about error handling and absolute paths is still applicable.
The current implementation using relative paths could fail if the script is run from the wrong directory or if required directories don't exist.
This PR adds argument files for the jpackage and jlink command.
Both java tools accept files containing arguments for the tool. Since the different builds share most of the jpackage/jlink arguments, this deduplicates a lot of code and increases maintainability.