-
-
Notifications
You must be signed in to change notification settings - Fork 196
Macos architecture matching installer #115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This commit introduces several optimizations to the `build-desktop-platforms.yml` GitHub Actions workflow: - **Enable build cancellation:** Adds a `concurrency` group to cancel in-progress runs on the same branch, saving CI resources. - **Introduce `setup-gradle` action:** Uses `gradle/actions/setup-gradle@v4` for more robust Gradle caching. - **Build macOS for arm64:** Adds a `macos-14` runner to the build matrix to generate native Apple Silicon (arm64) installers. - **Parallelize Gradle tasks:** Combines `package` tasks for each OS into a single command with the `--parallel` flag to speed up the build process. - **Enable artifact compression:** Sets the artifact `compression-level` to `6` to reduce storage usage and upload/download time.
The imports for `CpuArchitecture`, `jvmArchitecture`, and `osArchitecture` from `com.android.utils` have been removed as they were no longer in use.
Apps that have an update available will now be sorted to appear at the top of the list.
WalkthroughThis pull request optimizes the desktop build workflow by introducing concurrency control, matrix-based macOS builds, and consolidated parallel packaging tasks. It also updates desktop packaging metadata and modifies app list sorting order based on update availability status. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/build-desktop-platforms.yml (1)
74-82: Consider addingfail-fast: falsefor independent architecture builds.The matrix defaults to
fail-fast: true, meaning if one architecture fails, the other build is cancelled. For release artifacts, you typically want both builds to complete independently so you can still ship the successful one.Note: This produces separate architecture-specific installers (
macos-installers-x64andmacos-installers-arm64), not a single universal binary. If a true universal binary is desired, an additional step to combine the architectures usinglipowould be needed.🔎 Suggested fix to allow independent builds
strategy: + fail-fast: false matrix: include: - os: macos-13 arch: x64 - os: macos-14 arch: arm64
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build-desktop-platforms.ymlcomposeApp/build.gradle.ktscomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/apps/presentation/AppsViewModel.kt
🔇 Additional comments (7)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/apps/presentation/AppsViewModel.kt (1)
84-84: Verify the intended sort order for apps with updates.
sortedBy { it.installedApp.isUpdateAvailable }places apps without updates first (sincefalse < true). This pushes apps with available updates to the bottom of the list.Typically, apps requiring updates should be more prominent to encourage users to update. If the intent is to surface updatable apps first, use
sortedByDescendinginstead:🔎 Suggested fix if updatable apps should appear first
- }.sortedBy { it.installedApp.isUpdateAvailable } + }.sortedByDescending { it.installedApp.isUpdateAvailable }composeApp/build.gradle.kts (1)
230-230: Consider upgrade path implications from package name change.Changing
packageNamefrom"github-store"to"GitHub-Store"may affect upgrade detection on some platforms. Package managers on case-sensitive systems (macOS, Linux) might treat this as a different application, potentially causing:
- Side-by-side installations instead of upgrades
- Orphaned old installations requiring manual removal
If this is a first release or intentional rebrand, this is acceptable. Otherwise, verify that existing users can upgrade seamlessly.
.github/workflows/build-desktop-platforms.yml (5)
8-10: LGTM!Concurrency control prevents redundant workflow runs and conserves CI resources when new commits are pushed.
27-30: LGTM!The dedicated Gradle setup action provides better caching than the built-in Java action's Gradle cache, improving build times.
60-60: LGTM!Combining the packaging tasks with
--parallelimproves build efficiency by running both formats concurrently.
133-138: LGTM!Per-architecture artifact naming (
macos-installers-${{ matrix.arch }}) correctly distinguishes the matrix outputs, and compression level 6 provides good size reduction without excessive build time overhead.
154-158: LGTM!The Linux job follows the same patterns established in the Windows and macOS jobs (Gradle setup, parallel packaging, compression level), maintaining consistency across the workflow.
Also applies to: 186-186, 196-197
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.