refactor(macos): streamline KDF binary placement; update signing flow#247
refactor(macos): streamline KDF binary placement; update signing flow#247
Conversation
not only in "Release", runtime flags for signing in "Release", "Release-production", etc.
not only just "Release"
to avoid issues with unsigned resources ... we put SIGNED kdf binary to Contents/Frameworks/komodo_defi_framework.framework/Versions/Current/Helpers/kdf In case of using Resources - it will result with unsigned binary (!) TODO: May be this should be reworked, but we shouldn't store the binaries in resources, to avoid issues.
…cutable Eliminated the legacy path for the KDF executable in macOS to streamline the executable finding process. This change aligns with the recent restructuring of binary locations to avoid issues with unsigned resources.
Consolidated the creation of application support, frameworks, and helpers directories into a single command to simplify the build process.
|
Caution Review failedThe pull request is closed. 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. WalkthroughRefactors macOS executable discovery and packaging, transitioning from Resources bundle architecture to Helpers directory within the framework. Updates path construction in the finder and consolidates build-time code-signing operations into a reusable function. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The podspec contains dense, multi-faceted refactoring spanning resource bundling, helper directory setup, file-filtering logic, fat-binary pruning, and code-signing consolidation. The dart file presents straightforward path updates with consistent naming changes, but the heterogeneous nature of the build script modifications demands separate reasoning for each section. Poem
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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 |
There was a problem hiding this comment.
Pull Request Overview
Refactors macOS build/signing flow: relocates the kdf binary into the framework Helpers directory, updates pruning logic for Release configurations, and introduces a reusable code_sign_if_enabled function while removing the legacy resource bundle path in the Dart finder.
- Streamlines directory creation and replaces single Release equality check with pattern matching.
- Adds code signing helper and moves kdf lookup path to Framework Helpers.
- Removes old macOS resource bundle path from executable finder.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| macos/komodo_defi_framework.podspec | Adjusts resource bundles, directory setup, Release thinning logic, and introduces code_sign_if_enabled plus new Helpers placement and copy/sign flow. |
| lib/src/native/kdf_executable_finder.dart | Replaces legacy resource bundle path with new Helpers path inside the framework for macOS executable discovery. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| case "$CONFIGURATION" in | ||
| Release*) |
There was a problem hiding this comment.
The case statement is missing a pattern terminator (;;). Shell case syntax requires ';;' after the commands for the Release* branch; without it the script will error at 'esac'. Add ';;' after the last command inside the Release* section before line 101.
| thin_binary_to_archs "$FRAMEWORKS_DIR/libkdflib.dylib" "$TARGET_ARCHS" | ||
| if [ -f "$FRAMEWORKS_DIR/libkdflib.dylib" ]; then install_name_tool -id "@rpath/libkdflib.dylib" "$FRAMEWORKS_DIR/libkdflib.dylib"; fi | ||
| thin_binary_to_archs "$FRAMEWORKS_DIR/libkdflib.dylib" "$TARGET_ARCHS" | ||
| if [ -f "$FRAMEWORKS_DIR/libkdflib.dylib" ]; then install_name_tool -id "@rpath/libkdflib.dylib" "$FRAMEWORKS_DIR/libkdflib.dylib"; fi |
There was a problem hiding this comment.
The case statement is missing a pattern terminator (;;). Shell case syntax requires ';;' after the commands for the Release* branch; without it the script will error at 'esac'. Add ';;' after the last command inside the Release* section before line 101.
| if [ -f "$FRAMEWORKS_DIR/libkdflib.dylib" ]; then install_name_tool -id "@rpath/libkdflib.dylib" "$FRAMEWORKS_DIR/libkdflib.dylib"; fi | |
| if [ -f "$FRAMEWORKS_DIR/libkdflib.dylib" ]; then install_name_tool -id "@rpath/libkdflib.dylib" "$FRAMEWORKS_DIR/libkdflib.dylib"; fi | |
| ;; |
| # Helpers in komodo_defi_framework is now the ONLY place where KdfExecutableFinder.findExecutable() | ||
| # will look for the kdf binary on macOS. The APP_SUPPORT_DIR copy is redundant but kept for | ||
| # backward compatibility with older builds. | ||
| if [ -f "$APP_SUPPORT_DIR/kdf" ]; then cp "$APP_SUPPORT_DIR/kdf" "$HELPERS_DIR/kdf"; fi |
There was a problem hiding this comment.
The kdf binary is signed before being copied into Helpers, but the copy placed in Helpers (the only lookup location) is never re-signed. This can leave the executed binary unsigned, triggering Gatekeeper or runtime issues. Invoke code_sign_if_enabled on '$HELPERS_DIR/kdf' after the copy (or copy first, then sign both locations if backward compatibility is needed).
| if [ -f "$APP_SUPPORT_DIR/kdf" ]; then cp "$APP_SUPPORT_DIR/kdf" "$HELPERS_DIR/kdf"; fi | |
| if [ -f "$APP_SUPPORT_DIR/kdf" ]; then cp "$APP_SUPPORT_DIR/kdf" "$HELPERS_DIR/kdf"; fi | |
| code_sign_if_enabled "$HELPERS_DIR/kdf" || true |
| 'Resources', | ||
| 'Versions', | ||
| 'Current', | ||
| 'Helpers', |
There was a problem hiding this comment.
Bug: Dart Code Mismatch with Podspec Path
The Dart code expects the kdf binary within the framework at Versions/Current/Helpers/kdf, but the podspec copies it to Contents/Helpers/kdf. This path mismatch prevents the executable finder from locating the binary at runtime.
Additional Locations (1)
| 'kdf_resources' => ['lib/*.dylib'].select { |f| Dir.exist?(File.dirname(f)) } | ||
| } | ||
|
|
||
| # s.preserve_paths = ['bin/kdf'] |
There was a problem hiding this comment.
Bug: Missing kdf Executable in App Bundle
The bin/kdf executable is no longer included in resource_bundles (L19) and its s.preserve_paths directive is commented out (L22). This prevents CocoaPods from making the file available, causing the build script's copy operation (L40) to fail. Consequently, the kdf executable is missing from the app bundle, breaking runtime functionality.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Get the application support directory for macOS | ||
| APP_SUPPORT_DIR="${BUILT_PRODUCTS_DIR}/${PRODUCT_NAME}.app/Contents/Library/Application Support" | ||
| FRAMEWORKS_DIR="${BUILT_PRODUCTS_DIR}/${PRODUCT_NAME}.app/Contents/Frameworks" | ||
| HELPERS_DIR="${TARGET_BUILD_DIR}/${CONTENTS_FOLDER_PATH}/Helpers" | ||
|
|
||
| # Ensure the application support directory exists | ||
| if [ ! -d "$APP_SUPPORT_DIR" ]; then | ||
| mkdir -p "$APP_SUPPORT_DIR" | ||
| fi | ||
|
|
||
| # Ensure the frameworks directory exists | ||
| if [ ! -d "$FRAMEWORKS_DIR" ]; then | ||
| mkdir -p "$FRAMEWORKS_DIR" | ||
| fi | ||
| # Create all required directories in one go | ||
| mkdir -p "$APP_SUPPORT_DIR" "$FRAMEWORKS_DIR" "$HELPERS_DIR" | ||
|
|
There was a problem hiding this comment.
Copy kdf binary into framework Helpers path
The macOS script now defines HELPERS_DIR as ${TARGET_BUILD_DIR}/${CONTENTS_FOLDER_PATH}/Helpers and copies the executable there. However, kdf_executable_finder.dart was simultaneously updated to only look in Frameworks/komodo_defi_framework.framework/Versions/Current/Helpers/kdf. Because the script no longer places the binary inside the framework’s Helpers directory, the runtime search path no longer contains the copied file and the kdf executable cannot be found on macOS builds. Update HELPERS_DIR to point at ${FRAMEWORKS_DIR}/komodo_defi_framework.framework/Versions/Current/Helpers (or add that location to the finder) so the binary ends up where the Dart code expects it.
Useful? React with 👍 / 👎.
|
Visit the preview URL for this PR (updated for commit 342ca1f): https://komodo-playground--pr247-patch-macos-release-yvz2hf45.web.app (expires Mon, 27 Oct 2025 12:42:54 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 2bfedd77fdea45b25ba7c784416e81f177aa5c47 |
|
Visit the preview URL for this PR (updated for commit 342ca1f): https://kdf-sdk--pr247-patch-macos-release-plau059i.web.app (expires Mon, 27 Oct 2025 12:43:39 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 9c1b6e6c010cf0b965c455ba7a69c4aedafa8a1d |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Tested in GLEECBTC/gleec-wallet#3185 |
This PR merges macOS build/signing updates into dev.
Key changes
kdf_executable_finder.code_sign_if_enabledand apply signing in required configurations.thin_binary_to_archsfor all Release configs.Diff summary
Files touched
Notes
Summary by CodeRabbit