Feature: Split CI builds into different workflows #69
Conversation
WalkthroughThe pull request introduces significant changes to the GitHub Actions workflows for building a Java application across different operating systems. The existing workflow for Linux, Additionally, two new workflow files have been added: 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: 6
🧹 Outside diff range and nitpick comments (5)
.github/workflows/build-linux.yml (3)
36-42: Enhance error handling and input validationWhile the PowerShell script works correctly, consider these improvements:
- Add input validation for semantic versioning format
- Provide more descriptive error messages including expected format
- Write-Error "Version neither via input nor by tag specified. Aborting" + Write-Error "Version must be specified either via release tag or manual input (format: x.y.z). Neither was provided."
59-63: Consider templating artifact namesThe artifact naming pattern could be simplified by using a template variable in the matrix configuration.
include: - os: ubuntu-latest architecture: x64 native-access-lib: 'org.cryptomator.jfuse.linux.amd64' - artifact-name: cryptomator-cli-${{ needs.prepare.outputs.semVerStr }}-linux-x64.zip + arch-suffix: x64 - os: [self-hosted, Linux, ARM64] architecture: aarch64 native-access-lib: 'org.cryptomator.jfuse.linux.aarch64' - artifact-name: cryptomator-cli-${{ needs.prepare.outputs.semVerStr }}-linux-aarch64.zip + arch-suffix: aarch64Then use:
artifact-name: cryptomator-cli-${{ needs.prepare.outputs.semVerStr }}-linux-${{ matrix.arch-suffix }}.zip
Line range hint
1-132: Consider using reusable workflows for common stepsSince this is part of splitting CI builds into OS-specific workflows, consider extracting common steps (version determination, GPG signing, release publishing) into reusable workflows to maintain DRY principle and ease maintenance.
Example structure:
# .github/workflows/common/version.yml on: workflow_call: outputs: version: description: "Determined version" value: ${{ jobs.determine-version.outputs.version }} jobs: determine-version: # Current version determination logic.github/workflows/build-win.yml (1)
35-43: Enhance version determination robustnessThe PowerShell script could be improved by:
- Adding version format validation
- Handling empty strings explicitly
if ( '${{github.event_name}}' -eq 'release') { + if (-not '${{ github.event.release.tag_name}}') { + Write-Error "Release tag name is empty" + exit 1 + } + if (-not ('${{ github.event.release.tag_name}}' -match '^\d+\.\d+\.\d+(-\w+)?$')) { + Write-Error "Invalid version format. Expected: X.Y.Z[-suffix]" + exit 1 + } echo 'version=${{ github.event.release.tag_name}}' >> "$env:GITHUB_OUTPUT" exit 0 } elseif ('${{inputs.sem-version}}') { + if (-not ('${{inputs.sem-version}}' -match '^\d+\.\d+\.\d+(-\w+)?$')) { + Write-Error "Invalid version format. Expected: X.Y.Z[-suffix]" + exit 1 + } echo 'version=${{ inputs.sem-version}}' >> "$env:GITHUB_OUTPUT" exit 0 }.github/workflows/build-mac.yml (1)
34-43: Add semantic version validationThe version determination script should validate that the provided version follows semantic versioning format (X.Y.Z[-PRERELEASE][+BUILD]).
if ( '${{github.event_name}}' -eq 'release') { + if ('${{ github.event.release.tag_name}}' -notmatch '^\d+\.\d+\.\d+(?:-[\w-.]+)?(?:\+[\w-.]+)?$') { + Write-Error "Release tag does not follow semantic versioning" + exit 1 + } echo 'version=${{ github.event.release.tag_name}}' >> "$env:GITHUB_OUTPUT" exit 0 } elseif ('${{inputs.sem-version}}') { + if ('${{ inputs.sem-version}}' -notmatch '^\d+\.\d+\.\d+(?:-[\w-.]+)?(?:\+[\w-.]+)?$') { + Write-Error "Input version does not follow semantic versioning" + exit 1 + } echo 'version=${{ inputs.sem-version}}' >> "$env:GITHUB_OUTPUT" exit 0 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/build-linux.yml(6 hunks).github/workflows/build-mac.yml(1 hunks).github/workflows/build-win.yml(1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/build-mac.yml
78-78: shellcheck reported issue in this script: SC2086:info:1:1: Double quote to prevent globbing and word splitting
(shellcheck)
90-90: shellcheck reported issue in this script: SC2086:info:1:1: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/build-win.yml
70-70: shellcheck reported issue in this script: SC2086:info:1:1: Double quote to prevent globbing and word splitting
(shellcheck)
82-82: shellcheck reported issue in this script: SC2086:info:1:1: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (7)
.github/workflows/build-linux.yml (1)
85-89: Verify Java module list completeness
Please verify that the minimal set of Java modules includes all required dependencies. Current modules:
- java.base
- java.compiler
- java.naming
- java.xml
Consider if modules like java.logging or java.management might be needed for proper application functionality.
.github/workflows/build-win.yml (3)
17-18: Consider using a GA version of JDK
The workflow currently uses JDK 22.0.2+9 which is in early access. For production builds, it's recommended to use a GA (General Availability) version of the JDK to ensure stability.
Consider using JDK 21 LTS or waiting for JDK 22 GA release.
98-99: Review memory settings
The current memory settings might be restrictive:
- Stack size: 5MB
- Max heap: 256MB
Consider if these limits are sufficient for the application's needs, especially for processing large files.
70-81:
Fix path quoting in jlink command
The jlink command has unquoted paths which could cause issues with spaces in paths.
- --module-path "${JAVA_HOME}/jmods"
+ --module-path "\"${JAVA_HOME}/jmods\""Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint
70-70: shellcheck reported issue in this script: SC2086:info:1:1: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/build-mac.yml (3)
17-18: Verify Java version choice
The workflow uses Java 22.0.2+9 which is currently in early access. Consider using the latest LTS version (Java 21) for production builds unless there are specific features from Java 22 that are required.
52-62: LGTM! Well-structured matrix configuration
The matrix configuration properly handles both ARM64 and x64 architectures with appropriate runners and consistent artifact naming.
106-108: Review memory settings
The current memory settings (-Xmx256m) might be restrictive for some operations. Consider:
- Making this configurable via workflow inputs
- Increasing the default value based on typical usage patterns
- Adding minimum heap size (
-Xms) setting
| if-no-files-found: error | ||
| - name: Zip binary for release | ||
| if: startsWith(github.ref, 'refs/tags/') && github.event.action == 'published' | ||
| run: zip -r ./target/cryptomator-cli${{ matrix.binary-dir-suffix }} ./cryptomator-cli-${{ needs.prepare.outputs.semVerStr }}.zip | ||
| run: zip -r ./${{ matrix.artifact-name}} ./target/cryptomator-cli |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve file permissions in zip archive
The current zip command doesn't preserve executable permissions which might be crucial for Linux binaries.
- run: zip -r ./${{ matrix.artifact-name}} ./target/cryptomator-cli
+ run: zip -r --preserve-permissions ./${{ matrix.artifact-name}} ./target/cryptomator-cli📝 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: zip -r ./${{ matrix.artifact-name}} ./target/cryptomator-cli | |
| run: zip -r --preserve-permissions ./${{ matrix.artifact-name}} ./target/cryptomator-cli |
There was a problem hiding this comment.
The man page of zip does not mention this option. Also testing it on ubuntu 22.04 yields an error.
Hence, i assume, you are hallucinating this option.
There was a problem hiding this comment.
@infeo, you are absolutely right, and I apologize for the confusion. The zip command does not have a --preserve-permissions option, and it doesn't preserve Unix file permissions by default.
To ensure that file permissions are preserved in the archive, you might consider using tar instead of zip for the Linux build. The tar command preserves file permissions by default. Here's how you could modify the step:
- name: Tar binary for release
run: tar czvf ./${{ matrix.artifact-name}} ./target/cryptomator-cliThis will create a compressed .tar.gz archive (cryptomator-cli-x.y.z-linux-x64.tar.gz) that maintains the file permissions.
If distributing a .zip file is necessary, you might need to adjust the permissions after extraction or use a different tool that supports preserving permissions.
✏️ Learnings added
Learnt from: infeo
PR: cryptomator/cli#69
File: .github/workflows/build-linux.yml:117-117
Timestamp: 2024-11-20T12:01:28.237Z
Learning: The `zip` command does not have a `--preserve-permissions` option and does not preserve Unix file permissions. Use `tar` on Linux to archive files while preserving permissions.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.github/workflows/build-linux.yml (3)
Line range hint
16-17: Consider using major version for JavaInstead of pinning to a specific patch version
22.0.2+9, consider using just the major version22to automatically get the latest security updates while maintaining compatibility.env: JAVA_DIST: 'zulu' - JAVA_VERSION: '22.0.2+9' + JAVA_VERSION: '22'
84-89: Consider modularizing the required Java modulesThe
--add-moduleslist could be moved to a variable or property for better maintainability, especially if it needs to be updated in the future.env: REQUIRED_MODULES: 'java.base,java.compiler,java.naming,java.xml'Then use it in the jlink command:
--add-modules ${REQUIRED_MODULES}
118-123: Add error handling for GPG signingConsider adding error checking to ensure the GPG key import was successful before attempting to sign.
run: | echo "${GPG_PRIVATE_KEY}" | gpg --batch --quiet --import + if [ $? -ne 0 ]; then + echo "Failed to import GPG key" + exit 1 + fi echo "${GPG_PASSPHRASE}" | gpg --batch --quiet --passphrase-fd 0 --pinentry-mode loopback -u 615D449FE6E6A235 --detach-sign -a ./${{ matrix.artifact-name }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/build-linux.yml(5 hunks).github/workflows/build-mac.yml(1 hunks).github/workflows/build-win.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/build-mac.yml
- .github/workflows/build-win.yml
🧰 Additional context used
📓 Learnings (1)
.github/workflows/build-linux.yml (1)
Learnt from: infeo
PR: cryptomator/cli#69
File: .github/workflows/build-linux.yml:117-117
Timestamp: 2024-11-20T12:01:28.683Z
Learning: The `zip` command does not have a `--preserve-permissions` option and does not preserve Unix file permissions. Use `tar` on Linux to archive files while preserving permissions.
🔇 Additional comments (3)
.github/workflows/build-linux.yml (3)
35-42: LGTM! Clean version determination logic
The PowerShell script correctly handles both release tag and manual input scenarios with proper error handling.
Line range hint 54-64: LGTM! Well-structured matrix configuration
The matrix setup properly handles both x64 and ARM64 architectures with appropriate native access libraries and artifact naming.
116-116:
Replace zip with tar to preserve file permissions
Based on previous discussions, the zip command doesn't preserve Unix file permissions. For Linux builds, it's recommended to use tar instead.
- run: zip -r ./${{ matrix.artifact-name}} ./target/cryptomator-cli
+ run: tar czf ./${{ matrix.artifact-name}} -C ./target cryptomator-cliNote: This will require updating the artifact name to use .tar.gz extension in the matrix configuration.
This PR splits the CI binary/release builds into different workflow files.
The different OSs share a good amount of code, but on the other hand each has its peculiarities. Also Windows and macOS artifacts should be signed (not part of this PR).