-
-
Notifications
You must be signed in to change notification settings - Fork 157
chore: build and release updates #1476
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
1. remove default build for x86_64 apple 2. remove aarch64 kafka build for linux 3. add x86_64 kafka binary for linux 4. add aarch64 kafka binary for apple 5. remove default binary for x86_64 apple
WalkthroughThe CI workflows were simplified: the build workflow collapses Kafka cross-build complexity into a single Linux x86_64 path and a separate macOS aarch64 Kafka job; the release workflow adds explicit Kafka build jobs for Linux and macOS and wires them into artifact/checksum/release steps. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yaml(5 hunks).github/workflows/release.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1302
File: .github/workflows/build.yaml:170-175
Timestamp: 2025-04-26T03:58:02.341Z
Learning: In the parseable project, the Linux-specific environment variables (PKG_CONFIG_PATH, SASL2_DIR, OPENSSL_DIR, OPENSSL_ROOT_DIR, SASL2_STATIC) in the Kafka build step of GitHub Actions workflows don't cause issues for macOS builds and can safely be applied to both platforms.
📚 Learning: 2025-04-26T03:58:02.341Z
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1302
File: .github/workflows/build.yaml:170-175
Timestamp: 2025-04-26T03:58:02.341Z
Learning: In the parseable project, the Linux-specific environment variables (PKG_CONFIG_PATH, SASL2_DIR, OPENSSL_DIR, OPENSSL_ROOT_DIR, SASL2_STATIC) in the Kafka build step of GitHub Actions workflows don't cause issues for macOS builds and can safely be applied to both platforms.
Applied to files:
.github/workflows/build.yaml.github/workflows/release.yml
🪛 actionlint (1.7.9)
.github/workflows/release.yml
179-179: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
258-258: the runner of "actions/checkout@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: coverage
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
🔇 Additional comments (3)
.github/workflows/build.yaml (2)
28-31: LGTM — removal of x86_64 Apple build is sensible.The transition to aarch64-only for default builds aligns with modern macOS targets (M1/M2 and newer).
78-138: Kafka build simplification looks good; verify toolchain and profile alignment with release workflow.The consolidation to a fixed x86_64 Linux target and elimination of cross-compilation scaffolding is cleaner. However, note that this build uses
nightlyand the implicitreleaseprofile, while the new release workflow jobs (lines 198, 223 and 271, 295 inrelease.yml) usestableandrelease-lto. Confirm whether this intentional difference is acceptable or if alignment is needed..github/workflows/release.yml (1)
291-297: Verify environment variable handling for macOS Kafka build.The
build-kafka-linuxjob (lines 219-223) sets explicit environment variables (CC,CXX,LIBRDKAFKA_SSL_VENDORED) for the Kafka build, while thebuild-kafka-macjob (lines 292-296) only includesLIBRDKAFKA_SSL_VENDORED.Retrieved learnings from PR 1302 indicate that Linux-specific environment variables (
PKG_CONFIG_PATH,SASL2_DIR,OPENSSL_DIR,OPENSSL_ROOT_DIR,SASL2_STATIC) don't cause issues for macOS builds and can safely be applied to both platforms. However, the current approach omits them from the macOS build.Please confirm: Is this omission intentional (to keep the build configuration lean), or should these variables be included for consistency and to avoid potential linking issues on macOS?
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/release.yml (1)
351-352: Recommend standardizing checkout action version in Docker job.The
build-push-docker-imagejob (lines 348–403) usesactions/checkout@v3(line 352), while all other jobs in this workflow now use@v4. This appears to be a pre-existing inconsistency. Consider updating to@v4for consistency, though this is not a blocker for this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yaml(6 hunks).github/workflows/release.yml(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1302
File: .github/workflows/build.yaml:170-175
Timestamp: 2025-04-26T03:58:02.341Z
Learning: In the parseable project, the Linux-specific environment variables (PKG_CONFIG_PATH, SASL2_DIR, OPENSSL_DIR, OPENSSL_ROOT_DIR, SASL2_STATIC) in the Kafka build step of GitHub Actions workflows don't cause issues for macOS builds and can safely be applied to both platforms.
📚 Learning: 2025-04-26T03:58:02.341Z
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1302
File: .github/workflows/build.yaml:170-175
Timestamp: 2025-04-26T03:58:02.341Z
Learning: In the parseable project, the Linux-specific environment variables (PKG_CONFIG_PATH, SASL2_DIR, OPENSSL_DIR, OPENSSL_ROOT_DIR, SASL2_STATIC) in the Kafka build step of GitHub Actions workflows don't cause issues for macOS builds and can safely be applied to both platforms.
Applied to files:
.github/workflows/release.yml.github/workflows/build.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
🔇 Additional comments (9)
.github/workflows/build.yaml (3)
28-31: LGTM: Removes x86_64 macOS build from default matrix.The matrix now builds only the aarch64-apple-darwin target on macOS, aligning with the PR objective to remove the x86_64 Apple build and defer it to the dedicated Kafka job.
78-115: Kafka build job correctly isolated for Linux x86_64.The job explicitly targets x86_64-unknown-linux-gnu with appropriate dependencies, toolchain setup, and environment variables (CC=gcc-11, CXX=g++-11). The CMakeLists.txt patch (line 125) uses the correct sed syntax for Linux.
140-192: Verify macOS Kafka build environment consistency.The macOS Kafka job mirrors the Linux structure but intentionally omits CC/CXX environment variables (unlike the Linux job at lines 135-136). Confirm this is intentional and that the default clang toolchain is sufficient for the rdkafka build on macOS.
The sed syntax at line 181 correctly uses
-i ''for BSD sed (macOS), differentiating from the Linux variant at line 125. This is correct..github/workflows/release.yml (6)
35-35: LGTM: Checkout action updated to v4.The build-linux job now uses
actions/checkout@v4, addressing the previous deprecated v2 version.
81-81: LGTM: Checkout action updated to v4.The build-windows job now uses
actions/checkout@v4, maintaining consistency across all release jobs.
131-131: LGTM: Checkout action updated to v4.The build-mac job now uses
actions/checkout@v4, maintaining consistency across all release jobs.
171-247: Kafka Linux job correctly integrated into release workflow.The new
build-kafka-linuxjob (lines 171–247) mirrors the build.yaml Linux Kafka structure with appropriate dependencies, toolchain setup, and build configuration. Key observations:
- Uses
dtolnay/rust-toolchain@stable(same as other release jobs).- Employs
--profile release-ltofor optimized release builds (aligns with other release jobs at lines 47, 92, 144).- CMakeLists.txt patch syntax correct for Linux (line 211).
- Artifact naming and publishing follow the same pattern as other jobs.
This job addresses the PR objective to add x86_64 Kafka binary for Linux.
249-320: Kafka macOS job correctly integrated into release workflow.The new
build-kafka-macjob (lines 249–320) mirrors the build.yaml macOS Kafka structure with appropriate dependencies, toolchain setup, and build configuration. Key observations:
- Uses
dtolnay/rust-toolchain@stable(aligned with release workflow patterns).- Employs
--profile release-ltofor consistency with other release jobs.- Includes
stripcommand (line 296) for binary optimization, matching the build-mac pattern (line 145).- CMakeLists.txt patch syntax correct for macOS/BSD sed (line 284).
- Artifact naming and publishing follow the same pattern.
- Environment variables omit CC/CXX (intentional, using system clang), consistent with build.yaml.
This job addresses the PR objective to add aarch64 Kafka binary for Apple.
324-324: Dependency chain correctly updated.The
needsarray now includes both new Kafka jobs (build-kafka-linuxandbuild-kafka-mac), ensuring the create-checksum job waits for all build jobs to complete before aggregating checksums.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.