dev(nix): enable macOS cross-compilation#518
Conversation
Add support for downloading, installing, and running the Linaro compiler and patchelf toolchains on macOS (Darwin) hosts. Change-Id: c92ac710cc19af4426704c1619dd2918 Change-Id-Short: nqxpnsyznnyq
coderabbitai
Bot
added
the
dependencies
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔇 Additional comments (2)
WalkthroughAdds macOS support for Kobo cross-compilation: the Linaro toolchain derivation is platform-conditional for Darwin, devenv packaging and help text are updated, build tasks/scripts run unconditionally on Linux and macOS, and third-party build invocations receive explicit cross-compilation environment variables. ChangesKobo macOS Cross-Compilation Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xtask/src/tasks/build_kobo.rs (1)
9-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocstring is now outdated.
The module docstring still says the Linaro toolchain "is only available on Linux" and that "The task exits with a clear error on macOS." This no longer matches the implementation.
📝 Suggested fix
//! ## Platform requirement //! -//! Cross-compilation requires the Linaro ARM toolchain -//! (`arm-linux-gnueabihf-gcc` and friends) which is only available on Linux. -//! The task exits with a clear error on macOS. +//! Cross-compilation requires the Linaro ARM toolchain +//! (`arm-linux-gnueabihf-gcc` and friends) which is available on Linux and macOS. +//! On other platforms, the task exits with a clear error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xtask/src/tasks/build_kobo.rs` around lines 9 - 12, The module docstring at the top of build_kobo.rs is outdated: remove or rewrite the lines claiming the Linaro ARM toolchain "is only available on Linux" and that "The task exits with a clear error on macOS" so it matches the current behavior; update the docstring to state the actual requirement (cross-compilation needs the Linaro ARM toolchain) and that the task performs a runtime check and emits a helpful error if the toolchain is missing (refer to the top-of-file module docstring in build_kobo.rs to locate and edit the text).
🧹 Nitpick comments (1)
devenv.nix (1)
149-153: 💤 Low valueGoogle Drive URL may be unreliable for CI builds.
Google Drive download links can be rate-limited, require CAPTCHA, or change without notice. If CI starts failing intermittently, consider mirroring this artifact to a more stable host (GitHub Releases, S3, etc.).
That said, if this is the only available Darwin-compatible Linaro build, it's understandable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devenv.nix` around lines 149 - 153, The fetchurl entry using pkgs.fetchurl for the artifact named "gcc-linaro-darwin.tar.bz2" points at an unstable Google Drive download URL; replace this with a stable mirror (e.g., GitHub Releases, S3, or a canonical distro mirror) and update the url and sha256 values accordingly, or add a documented fallback/conditional that only uses the Google Drive URL if no stable mirror exists; ensure you update the pkgs.fetchurl block for the "gcc-linaro-darwin.tar.bz2" name and its sha256 to the values for the chosen mirror.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@xtask/src/tasks/util/thirdparty.rs`:
- Line 59: The CI requires rustfmt formatting for the command invocation:
reformat the call to cmd::output so it follows multiline formatting rules (split
the arguments across lines) and run cargo fmt; specifically update the
cmd::output("arm-linux-gnueabihf-readelf", &["-d", so_path_str], libs_dir, &[])?
invocation in xtask::tasks::util::thirdparty.rs to a properly formatted
multiline form and then run cargo fmt to persist changes.
---
Outside diff comments:
In `@xtask/src/tasks/build_kobo.rs`:
- Around line 9-12: The module docstring at the top of build_kobo.rs is
outdated: remove or rewrite the lines claiming the Linaro ARM toolchain "is only
available on Linux" and that "The task exits with a clear error on macOS" so it
matches the current behavior; update the docstring to state the actual
requirement (cross-compilation needs the Linaro ARM toolchain) and that the task
performs a runtime check and emits a helpful error if the toolchain is missing
(refer to the top-of-file module docstring in build_kobo.rs to locate and edit
the text).
---
Nitpick comments:
In `@devenv.nix`:
- Around line 149-153: The fetchurl entry using pkgs.fetchurl for the artifact
named "gcc-linaro-darwin.tar.bz2" points at an unstable Google Drive download
URL; replace this with a stable mirror (e.g., GitHub Releases, S3, or a
canonical distro mirror) and update the url and sha256 values accordingly, or
add a documented fallback/conditional that only uses the Google Drive URL if no
stable mirror exists; ensure you update the pkgs.fetchurl block for the
"gcc-linaro-darwin.tar.bz2" name and its sha256 to the values for the chosen
mirror.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b4d40f0-9517-49e0-9797-a728489c30dc
📒 Files selected for processing (6)
devenv.nixthirdparty/mupdf/build-kobo.shxtask/src/lib.rsxtask/src/tasks/build_kobo.rsxtask/src/tasks/setup_native.rsxtask/src/tasks/util/thirdparty.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Prefer?overunwrap()/expect()in library and app code
Usethiserrorfor custom error types andanyhowfor ad-hoc errors in Rust
Use iterators over index-based loops in Rust
Use&stroverStringin function parameters when ownership is not needed
Prefer borrowing over cloning in Rust
Inline expressions directly into struct fields—avoid intermediate bindings solely to pass them into a struct literal
Avoidunsafeunless required and documented in Rust
Avoid prematurecollect()in Rust—keep iterators lazy
Ensure Rust code compiles without warnings
Prefer newtype wrappers over raw primitives for domain concepts (e.g.,Fingerprint(String)instead of bareString)
WrapString,i64, and similar primitives when the value represents a specific domain concept (fingerprints, file kinds, identifiers, …)
ImplementDisplay,FromStr, and other standard traits on newtypes so they stay ergonomic
Match on enum variants instead of string comparisons—the compiler catches missing arms
Use structured fields with thetracingcrate in Rust—never use string formatting for log data
Use direct field formatting for primitives (int, float, bool) withtracingcrate
Use Display%formatter withtracingcrate for types implementingDisplay
Use Debug?formatter withtracingcrate for types implementingDebug
Usedebuglog level for development and troubleshooting detail
Useinfolog level for important runtime events
Usewarnlog level for recoverable issues (retries, fallbacks)
Useerrorlog level for failures requiring attention
Return domain types directly from sqlx queries instead of parsing primitives after the fact
Implementsqlx::Type<sqlx::Sqlite>,sqlx::Encode<sqlx::Sqlite>, andsqlx::Decode<sqlx::Sqlite>traits for custom types used in sqlx queries
Use sqlx type-override syntax with column aliases (e.g.,AS "fingerprint: Fp") so the macro deserializes into domain types directly
When the orphan rule prevents implem...
Files:
xtask/src/tasks/build_kobo.rsxtask/src/tasks/setup_native.rsxtask/src/lib.rsxtask/src/tasks/util/thirdparty.rs
🪛 GitHub Actions: Cargo / 69_format.txt
xtask/src/tasks/util/thirdparty.rs
[error] 56-59: cargo fmt --all --check failed (rustfmt formatting difference). Diff indicates missing multiline formatting for cmd::output(...) call around arm-linux-gnueabihf-readelf invocation.
🔇 Additional comments (10)
xtask/src/lib.rs (1)
24-24: LGTM!Also applies to: 74-74
devenv.nix (5)
23-37: LGTM!
155-176: LGTM!
317-322: LGTM!
684-688: LGTM!
712-716: LGTM!Also applies to: 792-801
xtask/src/tasks/build_kobo.rs (1)
103-108: LGTM!xtask/src/tasks/setup_native.rs (1)
60-60: LGTM!thirdparty/mupdf/build-kobo.sh (1)
6-6: LGTM!xtask/src/tasks/util/thirdparty.rs (1)
409-419: LGTM!
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
coderabbitai
Bot
removed
the
dependencies
|
@copilot can you address the following: Verify each finding against current code. Fix only still-valid issues, skip the In |
Agent-Logs-Url: https://github.com/OGKevin/cadmus/sessions/cf103815-eef9-4124-a27e-dec73cbf89ca Co-authored-by: OGKevin <17928966+OGKevin@users.noreply.github.com>
Addressed in d66ea45. I verified this finding is still valid and updated the top module docstring in |
coderabbitai
Bot
added
documentation
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xtask/src/tasks/build_kobo.rs (1)
99-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale
# Errorsrustdoc to match Linux+macOS support.Line 100 still says “platform is not Linux,” but
run()now rejects only non-Linux/non-macOS platforms. Please align the doc text with current behavior.Suggested doc fix
/// Returns an error if: -/// - The platform is not Linux. +/// - The platform is neither Linux nor macOS. /// - The Linaro toolchain is not on `PATH`. /// - Any build step fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xtask/src/tasks/build_kobo.rs` around lines 99 - 101, The rustdoc for build_kobo.rs is stale: update the `# Errors` section so it reflects that `run()` rejects non-Linux and non-macOS platforms (instead of only non-Linux); change the first bullet under "Returns an error if:" to mention "the platform is not Linux or macOS" (or equivalent wording) and keep the existing second bullet about the Linaro toolchain on `PATH`, ensuring the reference to the `run()` behavior and the error cases in that doc comment are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@xtask/src/tasks/build_kobo.rs`:
- Around line 99-101: The rustdoc for build_kobo.rs is stale: update the `#
Errors` section so it reflects that `run()` rejects non-Linux and non-macOS
platforms (instead of only non-Linux); change the first bullet under "Returns an
error if:" to mention "the platform is not Linux or macOS" (or equivalent
wording) and keep the existing second bullet about the Linaro toolchain on
`PATH`, ensuring the reference to the `run()` behavior and the error cases in
that doc comment are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 8997a658-0239-405e-bd5d-06cb210314a9
📒 Files selected for processing (1)
xtask/src/tasks/build_kobo.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: devenv (macos-latest)
- GitHub Check: build-docs-epub
- GitHub Check: devenv (ubuntu-latest)
- GitHub Check: Analyze (rust)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Prefer?overunwrap()/expect()in library and app code
Usethiserrorfor custom error types andanyhowfor ad-hoc errors in Rust
Use iterators over index-based loops in Rust
Use&stroverStringin function parameters when ownership is not needed
Prefer borrowing over cloning in Rust
Inline expressions directly into struct fields—avoid intermediate bindings solely to pass them into a struct literal
Avoidunsafeunless required and documented in Rust
Avoid prematurecollect()in Rust—keep iterators lazy
Ensure Rust code compiles without warnings
Prefer newtype wrappers over raw primitives for domain concepts (e.g.,Fingerprint(String)instead of bareString)
WrapString,i64, and similar primitives when the value represents a specific domain concept (fingerprints, file kinds, identifiers, …)
ImplementDisplay,FromStr, and other standard traits on newtypes so they stay ergonomic
Match on enum variants instead of string comparisons—the compiler catches missing arms
Use structured fields with thetracingcrate in Rust—never use string formatting for log data
Use direct field formatting for primitives (int, float, bool) withtracingcrate
Use Display%formatter withtracingcrate for types implementingDisplay
Use Debug?formatter withtracingcrate for types implementingDebug
Usedebuglog level for development and troubleshooting detail
Useinfolog level for important runtime events
Usewarnlog level for recoverable issues (retries, fallbacks)
Useerrorlog level for failures requiring attention
Return domain types directly from sqlx queries instead of parsing primitives after the fact
Implementsqlx::Type<sqlx::Sqlite>,sqlx::Encode<sqlx::Sqlite>, andsqlx::Decode<sqlx::Sqlite>traits for custom types used in sqlx queries
Use sqlx type-override syntax with column aliases (e.g.,AS "fingerprint: Fp") so the macro deserializes into domain types directly
When the orphan rule prevents implem...
Files:
xtask/src/tasks/build_kobo.rs
🔇 Additional comments (1)
xtask/src/tasks/build_kobo.rs (1)
11-13: LGTM!Also applies to: 104-108
Update soname() docs to name the actual binary used (arm-linux-gnueabihf-readelf, not generic readelf), and fix the run() # Errors section in build_kobo.rs to say Linux or macOS instead of Linux only. - build_kobo.rs: "The platform is not Linux or macOS." - thirdparty.rs: readelf -> arm-linux-gnueabihf-readelf in SONAMES constant doc and soname() function doc
coderabbitai
Bot
removed
documentation
OGKevin
added
documentation
|
aaah forgot about the damn docs 🫠 |
Add support for downloading, installing, and running the Linaro compiler and patchelf toolchains on macOS (Darwin) hosts.
Ref: baskerville/plato#417
Change-Id: c92ac710cc19af4426704c1619dd2918
Change-Id-Short: nqxpnsyznnyq