Skip to content

release/v1.6.2-dylib#626

Draft
neekolas wants to merge 9 commits intomainfrom
11-25-release_v1.6.2-dylib
Draft

release/v1.6.2-dylib#626
neekolas wants to merge 9 commits intomainfrom
11-25-release_v1.6.2-dylib

Conversation

@neekolas
Copy link
Contributor

release/v1.6.2-dylib

Revert "Remove dylib"

This reverts commit ff88ff5.

Copy link
Contributor Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

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

@claude
Copy link

claude bot commented Nov 26, 2025

Claude finished @neekolas's task —— View job


PR Review: release/v1.6.2-dylib

This PR reverts the removal of dylib support and configures the Swift package to use dynamic linking. Here's my analysis:


✅ Code Quality & Best Practices

Good:

  • Clean revert pattern with proper commit message referencing the original commit
  • Consistent naming convention change from LibXMTPSwiftFFILibXMTPSwiftFFIDynamic across all files
  • Version naming follows semantic versioning with -dylib suffix for clear differentiation

Observations:

  • The workflow file comments still reference LibXMTPSwiftFFI.xcframework but the code uses LibXMTPSwiftFFIDynamic.xcframework (Package.swift:57-67 and workflow:57-66). Consider updating the comment on line 57 of the workflow file for clarity.

🐛 Potential Issues

Critical:

  1. Inconsistent xcframework naming: The workflow extracts and bundles LibXMTPSwiftFFIDynamic.xcframework but the podspec still references LibXMTPSwiftFFI.xcframework at XMTP.podspec:28:

    spec.vendored_frameworks = 'LibXMTPSwiftFFI.xcframework'

    This mismatch will likely cause CocoaPods installation failures. The podspec should reference LibXMTPSwiftFFIDynamic.xcframework instead.

  2. Binary target URL validation: The new URL points to LibXMTPSwiftFFIDynamic.zip with a different checksum. Verify this artifact actually exists at the specified URL (Package.swift:29-30).

Medium:
3. Package.swift library type: The product is now explicitly set to .dynamic (Package.swift:12). This is intentional for this PR, but ensure downstream consumers are aware this changes the linking behavior and may affect app size/startup time.


⚡ Performance Considerations

Dynamic linking implications:

  • Startup time: Dynamic libraries are loaded at runtime, which may slightly increase app launch time compared to static linking
  • Memory: Dynamic libraries can be shared between processes, potentially reducing memory footprint if multiple processes use XMTP
  • Binary size: The main app binary will be smaller, but the total distribution size remains similar (dylib is separate)

For an SDK like XMTP, dynamic linking is reasonable if:

  • You expect the library to be updated independently
  • You're shipping via frameworks/XCFrameworks where dynamic is standard
  • You want to reduce main binary size

🔒 Security Concerns

Low risk, but verify:

  • The checksum change (Package.swift:30) is expected when switching binary variants, but verify the new checksum matches the actual artifact to prevent supply chain attacks
  • Ensure the libxmtp release at swift-bindings-1.6.3.ab0df09 includes both static and dynamic variants
  • The workflow uses --skip-import-validation (line 84) which bypasses some CocoaPods checks - ensure this is intentional

🧪 Test Coverage

Missing validation:

  • No automated tests verify the dynamic linking configuration
  • Consider adding a test that validates the library type in Package.swift matches expectations
  • The workflow should ideally test the built archive before publishing to CocoaPods

Recommendation:
Add a validation step in the workflow before the CocoaPods deployment:

- name: Validate archive structure
  run: |
    unzip -l XMTP-${{ env.UPDATED_VERSION }}.zip
    # Verify LibXMTPSwiftFFIDynamic.xcframework exists

🎯 Summary & Action Items

Must fix before merge:

  1. ⚠️ Update XMTP.podspec:28 to reference LibXMTPSwiftFFIDynamic.xcframework instead of LibXMTPSwiftFFI.xcframework

Recommended improvements:
2. Verify the binary artifact exists at the URL specified in Package.swift:29
3. Update workflow comment on line 57 to reflect LibXMTPSwiftFFIDynamic
4. Add archive validation step before CocoaPods deployment
5. Document the dynamic linking change in release notes for consumers

Questions to consider:

  • Is the switch to dynamic linking intended for all consumers or just specific use cases?
  • Have you tested CocoaPods installation with the -dylib version suffix?
  • Are there any integration tests that verify the dynamic framework loads correctly?

The changes are well-structured and the revert is clean, but the xcframework name mismatch in the podspec is a blocking issue that will break CocoaPods installations.


@neekolas neekolas mentioned this pull request Nov 26, 2025
@macroscopeapp
Copy link

macroscopeapp bot commented Nov 26, 2025

Configure SwiftPM product as dynamic and switch binary target to LibXMTPSwiftFFIDynamic for release/v1.6.2-dylib in Package.swift

Set product type: .dynamic, rename the binary target to LibXMTPSwiftFFIDynamic, and update its url and checksum in Package.swift; bump spec.version to 4.6.2-rc1-dylib in XMTP.podspec.

📍Where to Start

Start with the product and binary target configuration in Package.swift, then verify the version bump in XMTP.podspec.


Macroscope summarized ec4b34d.

@neekolas neekolas force-pushed the 11-25-release_v1.6.2-dylib branch 2 times, most recently from 11b0ca6 to 1a4135d Compare November 26, 2025 01:36
@neekolas neekolas force-pushed the 11-25-release_v1.6.2-dylib branch from 1a4135d to 4492fa5 Compare November 26, 2025 04:07
@neekolas neekolas force-pushed the 11-25-release_v1.6.2-dylib branch from 4492fa5 to 3f86791 Compare November 26, 2025 04:32
@neekolas neekolas force-pushed the 11-25-release_v1.6.2-dylib branch 2 times, most recently from 555061d to 11a7318 Compare November 26, 2025 21:44
@neekolas neekolas force-pushed the release/v1.6.2 branch 2 times, most recently from 2681f5b to aa524d5 Compare November 26, 2025 23:29
@neekolas neekolas force-pushed the 11-25-release_v1.6.2-dylib branch 3 times, most recently from f831d11 to 4862bde Compare November 26, 2025 23:58
@neekolas neekolas force-pushed the 11-25-release_v1.6.2-dylib branch from 4862bde to ec0e56c Compare November 30, 2025 02:32
Base automatically changed from release/v1.6.2 to main December 2, 2025 23:51
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.

3 participants