Skip to content

dynamic library bindings#603

Open
insipx wants to merge 1 commit intomainfrom
insipx/dynamic-swift
Open

dynamic library bindings#603
insipx wants to merge 1 commit intomainfrom
insipx/dynamic-swift

Conversation

@insipx
Copy link

@insipx insipx commented Oct 23, 2025

Add dynamic library product XMTPiOS in Package.swift to provide dynamic bindings and mark existing library product as .static

Introduce a dynamic library product and target with a dynamic FFI dependency in Package.swift.

  • Add XMTPiOS as a dynamic library product backed by target XMTPiOSDynamic
  • Create target XMTPiOSDynamic with dependencies on Connect from connect-swift, binary target LibXMTPSwiftFFIDynamic, and CryptoSwift; path Sources/XMTPiOS
  • Declare binary target LibXMTPSwiftFFIDynamic with URL and checksum
  • Set existing library product type to .static

📍Where to Start

Start with the product and target definitions in Package.swift, focusing on XMTPiOS and XMTPiOSDynamic and the LibXMTPSwiftFFIDynamic binary target declaration.


Macroscope summarized 4dbd63e.

@insipx insipx marked this pull request as ready for review October 23, 2025 13:41
@insipx insipx requested a review from a team as a code owner October 23, 2025 13:41
Copy link
Author

insipx commented Oct 23, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Oct 23, 2025

Claude finished @insipx's task —— View job


Code Review Complete

Tasks

  • Read and analyze Package.swift changes
  • Review code quality and Swift Package Manager best practices
  • Check for potential bugs and compatibility issues
  • Evaluate performance considerations
  • Review security concerns
  • Assess test coverage needs
  • Provide comprehensive feedback

Review Feedback

I've analyzed the changes in Package.swift that introduce dynamic library support. Here's my detailed feedback:

🔴 Critical Issues

1. Duplicate Product Name (Package.swift:10-23)

Severity: High - Build Error

.library(
    name: "XMTPiOS",
    type: .static,
    targets: ["XMTPiOS"]
),
// ... 
.library(
    name: "XMTPiOS",  // ❌ Duplicate product name
    type: .dynamic,
    targets: ["XMTPiOSDynamic"]
)

Issue: Both products have the same name "XMTPiOS". Swift Package Manager does not allow duplicate product names in the same package manifest. This will cause a build error when consumers try to use this package.

Impact:

  • Package will fail to build
  • SPM will throw an error: "duplicate product named 'XMTPiOS'"
  • Consumers won't be able to resolve dependencies

Recommendation: Rename the dynamic product to distinguish it from the static one:

.library(
    name: "XMTPiOSDynamic",  // or "XMTPiOS-Dynamic"
    type: .dynamic,
    targets: ["XMTPiOSDynamic"]
)

⚠️ Major Concerns

2. Source Path Sharing (Package.swift:47-54)

Severity: Medium

The XMTPiOSDynamic target uses path: "Sources/XMTPiOS", which is the same source directory as the static XMTPiOS target.

Potential Issues:

  • Both targets compile the same source files
  • May cause conflicts or unexpected behavior with module caching
  • Could lead to symbol conflicts if both libraries are linked in the same app

Questions to validate:

  • Is this intentional to avoid code duplication?
  • Have you tested that both products can coexist in the same dependency graph?
  • Are there any build settings that differ between the two targets?

3. Missing Binary Target Validation (Package.swift:55-60)

Severity: Medium - Security

The new binary target LibXMTPSwiftFFIDynamic references a development release:

url: "https://github.com/xmtp/libxmtp/releases/download/swift-bindings-dynamic-1.6.0-dev.ca1e5bf/LibXMTPSwiftFFIDynamic.zip"

Concerns:

  • Dev release (1.6.0-dev.ca1e5bf) - is this intended for production?
  • Different release than the static version (swift-bindings-1.6.0-dev.3656d63 vs swift-bindings-dynamic-1.6.0-dev.ca1e5bf)
  • No way to verify the binary hasn't been tampered with beyond the checksum
  • Checksum is provided, which is good, but users can't easily verify its correctness

Recommendations:

  • Document why dynamic and static versions use different libxmtp releases
  • Consider using a stable release version instead of dev builds
  • Add a comment explaining the checksum verification process

4. Test Coverage (Package.swift:65-68)

Severity: Medium

The test target only depends on the static XMTPiOS:

.testTarget(
    name: "XMTPTests",
    dependencies: ["XMTPiOS", "XMTPTestHelpers"]
)

Issue: No tests verify the dynamic library product works correctly.

Recommendations:

  • Add a separate test target for the dynamic library: XMTPTestsDynamic
  • Verify symbol visibility and linking behavior
  • Test that the dynamic library works in both iOS and macOS contexts
  • Ensure there are no runtime issues specific to dynamic linking

💡 Best Practices & Suggestions

5. Documentation Missing

Severity: Low

There's no documentation explaining:

  • When to use the static vs dynamic library
  • Performance implications of each choice
  • Binary size considerations
  • Whether mixing both in one app is supported

Recommendation: Add comments in Package.swift or update README.md with guidance.

6. Dependency Consistency

Severity: Low

Both targets depend on the same packages (Connect, CryptoSwift), which is correct. However, verify that:

  • These dependencies support both static and dynamic linking
  • There won't be duplicate symbol issues if both products are used together

🔒 Security Assessment

Good: Checksums are provided for both binary targets
Good: Using HTTPS URLs for downloads
⚠️ Concern: Development releases instead of stable versions
⚠️ Concern: No documentation about binary provenance

⚡ Performance Considerations

  • Dynamic linking: Slower launch time, but smaller app size if the library is shared
  • Static linking: Faster launch time, but larger binary size
  • Trade-off: Good to offer both options, but needs documentation

📋 Summary

Blocking Issues (must fix):

  1. ❌ Duplicate product name will cause build failure

Recommended Changes:
2. ✏️ Add test coverage for dynamic library
3. ✏️ Document when to use static vs dynamic
4. ✏️ Explain why different libxmtp versions are used
5. ✏️ Consider moving to stable releases instead of dev builds

Action Items:

  • Rename the dynamic product to avoid naming conflict
  • Add tests for the dynamic library variant
  • Document the usage and differences in README or inline comments
  • Validate that both products can coexist (or document that they can't)

@insipx insipx force-pushed the insipx/dynamic-swift branch 4 times, most recently from d56e9b1 to 585ed90 Compare October 23, 2025 14:02
@insipx insipx changed the title dynamic swift dynamic library bindings Oct 23, 2025
@insipx insipx force-pushed the insipx/dynamic-swift branch 3 times, most recently from 3ede6c8 to 76a821d Compare October 24, 2025 23:15
@insipx insipx force-pushed the insipx/dynamic-swift branch from 76a821d to aabd5ca Compare October 24, 2025 23:19
@insipx insipx force-pushed the insipx/dynamic-swift branch from aabd5ca to 4dbd63e Compare October 24, 2025 23:29
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.

1 participant