fix(build): build on macos #1770
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds Darwin/macOS detection to shared Makefile, selects platform-appropriate shared library extension/name, introduces a macOS codesign macro, updates Makefiles and cgo linker flags to use the platform-aware library name, adds cdylib output for the Rust crate, and signs macOS-built binaries. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Make as Makefile (includes build/common.mk)
participant Toolchain as Build Toolchain
participant Codesign as macOS codesign
Note over Make: determine SHLIB_EXT & LIB_ZKP_NAME\ndefine macos_codesign macro
Dev->>Make: invoke target (e.g., coordinator_api / test_tool)
Make->>Toolchain: build binary (and produce lib/$(LIB_ZKP_NAME))
alt IS_DARWIN == true
Make->>Codesign: call macos_codesign(<binary>)
Codesign-->>Make: codesign --force --sign -\ncodesign --verify --deep --verbose
else IS_DARWIN == false
Note right of Make: codesign step skipped
end
Make->>Dev: artifact(s) produced
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
🧹 Nitpick comments (5)
coordinator/.gitignore (1)
4-5: Darwin artifact ignore looks good; consider a wildcard for future extensionsAdding
libzkp.dylibkeeps macOS build artifacts out of git. You might also consider a single pattern likeinternal/libzkp/lib/libzkp.*to cover both.soand.dylib(and any future variants) instead of two separate lines.coordinator/README.md (1)
13-18: Clarify Go version requirement in a tool-agnostic wayThe
gvm use go1.22hint is helpful, but a brief preceding comment like “Go 1.22 is required (example using gvm below)” would make it clearer that any version manager (or system install) satisfying 1.22 is fine, not just gvm.coordinator/internal/logic/libzkp/Makefile (1)
1-2: Keepcleanin sync withLIB_ZKP_NAMEfor Darwin buildsIncluding
common.mkand switching the copy step to$(LIB_ZKP_NAME)is the right direction for cross‑platform builds. Thecleantarget still hardcodeslib/libzkp.so, though, so on macOS thelibzkp.dylibwill survive amake clean.Consider updating the rule to use the same variable:
clean: - @cargo clean --release -p libzkp -p libzkp-c -p l2geth - @rm -f lib/libzkp.so + @cargo clean --release -p libzkp -p libzkp-c -p l2geth + @rm -f lib/$(LIB_ZKP_NAME)or, if you want to be extra safe across historical layouts:
@rm -f lib/libzkp.so lib/libzkp.dylibAlso applies to: 5-9, 13-16
coordinator/Makefile (1)
1-2: Common lib naming and macOS codesign hook look good; please verify ignore pathsUsing
../build/common.mkplusLIBZKP_PATH=./internal/logic/libzkp/lib/$(LIB_ZKP_NAME)ties the coordinator build cleanly into the sharedSHLIB_EXT/LIB_ZKP_NAMElogic, and themacos_codesigncall oncoordinator_apiis correctly guarded by the Darwin check incommon.mk.One thing to double‑check:
coordinator/.gitignoreis still ignoringinternal/libzkp/lib/libzkp.{so,dylib}(without thelogicsegment), while this Makefile now expects the library underinternal/logic/libzkp/lib. If the old path is no longer used, updating the ignore patterns to match the actual output directory would avoid accidentally tracking built libs.Also applies to: 5-8, 30-33
build/common.mk (1)
1-16: Shared Darwin handling andmacos_codesignhelper are well-structuredCentralizing
UNAME_S,SHLIB_EXT, andLIB_ZKP_NAMEhere, plus themacos_codesignmacro guarded byIS_DARWIN, is a clean way to unify platform behavior across Makefiles. This should fix straightforward macOS local builds without impacting Linux.If you ever need cross‑compilation (e.g., building Linux artifacts on macOS), you might later consider making
UNAME_S/SHLIB_EXToverridable via the environment, but that can easily be deferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
build/common.mk(1 hunks)coordinator/.gitignore(1 hunks)coordinator/Makefile(2 hunks)coordinator/README.md(1 hunks)coordinator/internal/logic/libzkp/Makefile(1 hunks)coordinator/internal/logic/libzkp/lib.go(1 hunks)crates/libzkp/Cargo.toml(1 hunks)tests/prover-e2e/Makefile(3 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
coordinator/internal/logic/libzkp/Makefile
[warning] 3-3: Missing required phony target "all"
(minphony)
[warning] 3-3: Missing required phony target "clean"
(minphony)
coordinator/Makefile
[warning] 3-3: Missing required phony target "all"
(minphony)
[warning] 3-3: Missing required phony target "test"
(minphony)
⏰ 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). (3)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (3)
crates/libzkp/Cargo.toml (1)
6-8: Enabling bothrlibandcdyliblooks appropriateDeclaring
crate-type = ["rlib", "cdylib"]aligns with using this crate both as a Rust dependency and as a C‑compatible shared library backing the coordinator/libzkp bindings. No further changes needed from my perspective.tests/prover-e2e/Makefile (1)
1-2: macOS codesign integration for e2e_tool is wired correctlyIncluding
../../build/common.mkand then calling$(call macos_codesign,$(PWD)/build/bin/e2e_tool)cleanly reuses the shared Darwin logic while remaining a no‑op on non‑macOS hosts. Thecoordinator_setupcopy into../../coordinator/build/bin/confalso matches whatlocalsetupprepares, so this target’s flow looks sound.Also applies to: 55-58, 75-77
coordinator/internal/logic/libzkp/lib.go (1)
3-6: I'll verify the review comment by examining the actual file and checking whether the concerns about macOS compatibility and-ldllinking are valid.
<function_calls>
#!/bin/bashFirst, let's read the actual file to see the current state
cat -n coordinator/internal/logic/libzkp/lib.go | head -20
</function_calls>I need to check the file first. Let me use the appropriate capability:
<function_calls>
#!/bin/bashFind the file and read it
find . -name "lib.go" -path "/libzkp/" | head -5
</function_calls>Let me search more carefully for this file:
<function_calls>
#!/bin/bashSearch for the file
fd "lib.go" | grep -i libzkp
</function_calls>Let me try a broader search:
<function_calls>
#!/bin/bashList files in the coordinator directory
find . -type d -name "libzkp" 2>/dev/null
find . -path "coordinator" -name "lib.go" 2>/dev/null | head -10
</function_calls><function_calls>
#!/bin/bashSearch for any cgo comments with LDFLAGS
rg -l "#cgo LDFLAGS" --type go
</function_calls><function_calls>
#!/bin/bashLook for cgo directives with -ldl
rg "-ldl" --type go -B2 -A2
</function_calls>
There was a problem hiding this comment.
Pull request overview
This PR enables building the project on macOS by addressing platform-specific library and code signing requirements. The changes add support for macOS's .dylib library format and implement automatic code signing for binaries on Darwin systems.
Key Changes
- Introduced platform detection and conditional library extension handling (
.sovs.dylib) - Added automated macOS code signing for built binaries
- Updated Rust library configuration to support dynamic library builds
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| build/common.mk | New shared makefile with platform detection, library extension selection, and macOS code signing function |
| coordinator/Makefile | Includes common.mk and applies dynamic library naming and code signing to coordinator_api binary |
| coordinator/internal/logic/libzkp/Makefile | Uses dynamic library name from common.mk instead of hardcoded .so extension |
| coordinator/internal/logic/libzkp/lib.go | Fixed LDFLAGS syntax for rpath to be macOS-compatible |
| coordinator/.gitignore | Added .dylib to ignored files for macOS library builds |
| crates/libzkp/Cargo.toml | Added cdylib crate type to support dynamic library generation |
| tests/prover-e2e/Makefile | Includes common.mk and applies code signing to test tool binary |
| coordinator/README.md | Added optional Go version comment for build instructions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
coordinator/README.md(1 hunks)coordinator/internal/logic/libzkp/Makefile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- coordinator/README.md
🧰 Additional context used
🪛 checkmake (0.2.2)
coordinator/internal/logic/libzkp/Makefile
[warning] 3-3: Missing required phony target "all"
(minphony)
[warning] 3-3: Missing required phony target "clean"
(minphony)
⏰ 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). (3)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (3)
coordinator/internal/logic/libzkp/Makefile (3)
1-1: Platform-aware library naming infrastructure is properly initialized.The inclusion of the shared
build/common.mkenables platform detection for macOS and other operating systems.
8-8: Platform-aware library naming correctly replaces hardcoded .so extension.Using
$(LIB_ZKP_NAME)instead of the hardcodedlibzkp.soallows the build system to select the appropriate library extension (.dylibon macOS,.soon Linux, etc.) consistently across build and clean targets.Also applies to: 15-15
1-1: The variableLIB_ZKP_NAMEis properly defined inbuild/common.mkand correctly used.Verification confirms that
LIB_ZKP_NAMEis defined at line 9 ofbuild/common.mkaslibzkp.$(SHLIB_EXT), withSHLIB_EXTcorrectly set tosofor Linux (line 4) anddylibfor macOS (lines 5-7). The variable is actively used in both the build target (line 8) and clean target (line 15) of the included Makefile. No issues found.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1770 +/- ##
===========================================
- Coverage 36.53% 36.47% -0.07%
===========================================
Files 247 247
Lines 21188 21188
===========================================
- Hits 7742 7728 -14
- Misses 12616 12637 +21
+ Partials 830 823 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Purpose or design rationale of this PR
This PR enables building the project on macOS by addressing platform-specific library and code signing requirements. The changes add support for macOS's .dylib library format and implement automatic code signing for binaries on Darwin systems.
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.