Skip to content

Conversation

@dlipicar
Copy link
Contributor

Bump go-wallet-sdk
Link the lib statically instead of dynamically.

Copilot AI review requested due to automatic review settings December 11, 2025 15:25
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR converts the go-wallet-sdk dependency from dynamic linking to static linking, eliminating the need to distribute separate shared library files with the wallet module plugin.

Key Changes

  • Updates go-wallet-sdk to commit 0938a704 which includes static library build support
  • Builds libgowalletsdk.a static library instead of shared libraries (.so, .dylib)
  • Adds platform-specific whole-archive linker flags and macOS framework dependencies required for static linking
  • Removes dynamic library installation and install_name_tool fix-up logic that is no longer needed

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
flake.nix Updates go-wallet-sdk dependency to newer commit with static linking support
flake.lock Updates lock file to match new go-wallet-sdk commit hash and metadata
nix/lib.nix Changes build to produce static library (.a) instead of shared library (.so/.dylib)
metadata.json Removes dynamic library files from include list since they're no longer shipped
fix_wallet_install_name.cmake Removes entire file as install_name_tool fixes are unnecessary with static linking
CMakeLists.txt Updates library search, linking configuration with whole-archive flags, and macOS framework dependencies; removes dynamic library copying and install_name_tool commands
Comments suppressed due to low confidence (1)

nix/lib.nix:113

  • Since the go-wallet-sdk library is now statically linked into the wallet_module_plugin, copying the static library file libgowalletsdk.a to the output directory may be unnecessary. Static libraries are compiled into the binary at link time and don't need to be distributed separately. Consider removing this installation step unless there's a specific need to distribute the static library for other purposes.
    # Copy the wallet library if it exists (check both lib/ and modules/ directories)
    if [ -f lib/libgowalletsdk.a ]; then
      cp lib/libgowalletsdk.a $out/lib/
      echo "Copied libgowalletsdk.a from lib/ to $out/lib/"
    elif [ -f modules/libgowalletsdk.a ]; then
      cp modules/libgowalletsdk.a $out/lib/
      echo "Copied libgowalletsdk.a from modules/ to $out/lib/"
    else
      echo "Warning: No wallet library found in lib/ or modules/"
      echo "Contents of lib/:"
      ls -la lib/ 2>/dev/null || echo "lib/ directory does not exist"
      echo "Contents of modules/:"
      ls -la modules/ 2>/dev/null || echo "modules/ directory does not exist"
    fi

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +212 to +213
target_link_libraries(wallet_module_plugin PUBLIC "-framework CoreFoundation")
target_link_libraries(wallet_module_plugin PUBLIC "-framework Security")
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The macOS frameworks CoreFoundation and Security are linked with PUBLIC visibility, which exports these dependencies to consumers of wallet_module_plugin. Since this appears to be a plugin library and these frameworks are implementation details needed for the static go-wallet-sdk library, consider using PRIVATE visibility instead to avoid exposing these dependencies unnecessarily.

Suggested change
target_link_libraries(wallet_module_plugin PUBLIC "-framework CoreFoundation")
target_link_libraries(wallet_module_plugin PUBLIC "-framework Security")
target_link_libraries(wallet_module_plugin PRIVATE "-framework CoreFoundation")
target_link_libraries(wallet_module_plugin PRIVATE "-framework Security")

Copilot uses AI. Check for mistakes.
endif()
add_custom_command(TARGET wallet_module_plugin POST_BUILD
COMMAND install_name_tool -id "@rpath/wallet_module_plugin.dylib" $<TARGET_FILE:wallet_module_plugin>
COMMENT "Updating library paths for macOS (accounts library not found)"
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment mentions "accounts library not found" but should refer to the wallet library. This appears to be a copy-paste error from another part of the codebase.

Suggested change
COMMENT "Updating library paths for macOS (accounts library not found)"
COMMENT "Updating library paths for macOS (wallet library not found)"

Copilot uses AI. Check for mistakes.
Comment on lines +219 to +227
target_link_libraries(wallet_module_plugin PRIVATE ${LIBGOWALLETSDK})

if(APPLE)
# macOS specific settings
target_link_options(wallet_module_plugin PRIVATE -Wl,-force_load ${LIBGOWALLETSDK})
else()
# Linux specific settings
target_link_options(wallet_module_plugin PRIVATE -Wl,--whole-archive ${LIBGOWALLETSDK} -Wl,--no-whole-archive)
endif()
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The static library is linked twice: once on line 219 via target_link_libraries, and again via target_link_options with whole-archive flags on lines 223/226. This redundant linking should be removed. Consider removing line 219 and keeping only the whole-archive linking, or use only the basic linking if the whole-archive approach is not needed.

Copilot uses AI. Check for mistakes.
@dlipicar dlipicar marked this pull request as draft December 11, 2025 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants