Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Figure out an alternative way to handle the Rust components used by ZcashLightClientKit #96

Open
str4d opened this issue Aug 29, 2023 · 8 comments

Comments

@str4d
Copy link
Contributor

str4d commented Aug 29, 2023

The sizes of the binaries in this repo have been slowly growing over time. GitHub warns if any binary artifact in a commit is over 50 MiB, which the simulator artifact is, and GitHub rejects any commit containing a binary artifact over 100 MiB, which the simulator artifact is when compiling with ThinLTO.

We will need to figure out an alternative strategy for handling these binary artifacts. If/when we move to using UniFFI instead of the manual FFI in this repo, then this repo will be purely a cache of the binary artifacts, so we may be able to cache them in a different way.

@str4d
Copy link
Contributor Author

str4d commented Aug 29, 2023

@zookozcash

@str4d
Copy link
Contributor Author

str4d commented Sep 27, 2023

I just updated a clone of this repository that was previously at 0.3.1, and is now at 0.4.0:

git pull
❯ git pull
remote: Enumerating objects: 163, done.
remote: Counting objects: 100% (92/92), done.
remote: Compressing objects: 100% (48/48), done.
remote: Total 163 (delta 44), reused 63 (delta 29), pack-reused 71
Receiving objects: 100% (163/163), 248.57 MiB | 10.16 MiB/s, done.
Resolving deltas: 100% (63/63), completed with 2 local objects.
From https://github.com/zcash-hackworks/zcash-light-client-ffi
   75821e2..aa4c0b1  main       -> origin/main
 * [new tag]         0.4.0      -> 0.4.0
 * [new tag]         0.4.0-rc.1 -> 0.4.0-rc.1
 * [new tag]         0.4.0-rc.2 -> 0.4.0-rc.2
 * [new tag]         0.4.0-rc.3 -> 0.4.0-rc.3
 * [new tag]         0.4.0-rc.4 -> 0.4.0-rc.4
Updating 75821e2..aa4c0b1
Fast-forward
 CHANGELOG.md                                                                                 |   80 ++-
 releases/XCFramework/libzcashlc.xcframework/ios-arm64/libzcashlc.framework/Headers/zcashlc.h | 1130 +++++++++++++++++++++------------------
 releases/XCFramework/libzcashlc.xcframework/ios-arm64/libzcashlc.framework/libzcashlc        |  Bin 27382872 -> 47650424 bytes
 .../libzcashlc.xcframework/ios-arm64_x86_64-simulator/libzcashlc.framework/Headers/zcashlc.h | 1130 +++++++++++++++++++++------------------
 .../libzcashlc.xcframework/ios-arm64_x86_64-simulator/libzcashlc.framework/libzcashlc        |  Bin 42960984 -> 71066888 bytes
 .../libzcashlc.xcframework/macos-arm64_x86_64/libzcashlc.framework/Headers/zcashlc.h         | 1130 +++++++++++++++++++++------------------
 .../XCFramework/libzcashlc.xcframework/macos-arm64_x86_64/libzcashlc.framework/libzcashlc    |  Bin 21339216 -> 33112064 bytes
 rust/Cargo.lock                                                                              | 1253 ++++++++++++++++++++------------------------
 rust/Cargo.toml                                                                              |   43 +-
 rust/build.rs                                                                                |   21 +-
 rust/src/ffi.rs                                                                              |    2 +
 rust/src/lib.rs                                                                              | 1176 +++++++++++++++++++++++------------------
 rust/src/os_log.rs                                                                           |   80 +++
 rust/src/os_log/layer.rs                                                                     |   32 ++
 rust/src/os_log/signpost.rs                                                                  |   96 ++++
 rust/src/os_log/writer.rs                                                                    |   99 ++++
 rust/wrapper.c                                                                               |   16 +
 rust/wrapper.h                                                                               |    7 +
 18 files changed, 3535 insertions(+), 2760 deletions(-)
 create mode 100644 rust/src/ffi.rs
 create mode 100644 rust/src/os_log.rs
 create mode 100644 rust/src/os_log/layer.rs
 create mode 100644 rust/src/os_log/signpost.rs
 create mode 100644 rust/src/os_log/writer.rs
 create mode 100644 rust/wrapper.c
 create mode 100644 rust/wrapper.h
Target 0.3.1 (MiB) 0.4.0 (MiB) Increase
ios-arm64 26.11 45.44 74.0%
ios-arm64_x86_64-simulator 40.97 67.77 65.4%
macos-arm64_x86_64 20.35 31.58 55.2%

This is definitely unsustainable. It also interferes with the ability to use ZcashLightClientKit as a Swift library (because this dependency hard-codes Apple-specific binaries).

@str4d
Copy link
Contributor Author

str4d commented Sep 27, 2023

The current architecture is:

  • zcash-light-client-ffi has a Package.swift that includes a binaryTarget referencing the checked-in files.
  • ZcashLightClientKit has a package pointing at zcash-light-client-ffi in the dependencies section of its Package.swift.

binaryTarget can reference a remote URL with a checksum instead of a local path, so one option is to change the release process to upload the built files as part of a tagged release. That wouldn't reduce the current size of the repository or help with past deployments, but it would stop the repository size from growing further (instead using GitHub tagged releases for binary distribution).

If we keep the Package.swift in this repository then there is a tagging dance that needs to happen, where we would need to:

  • Build the binary artifacts.
  • Update Package.swift to reference them, including checksum.
  • Tag the release.
  • Upload the binary artifacts to the release.

The other thing the documentation says is that binary targets are only available on Apple platforms, so if we do want to enable ZcashLightClientKit to be used as a general Swift library, we need the binary targets to be conditional, and the source code to be used instead for other targets (i.e. having rustc in the build process for ZcashLightClientKit on non-Apple targets). It looks like as of XCode 14.3 binary targets can be conditional, so we might be able to structure things such that for Apple targets a binary target is used, and for non-Apple targets the Rust builds are done in-line.

@str4d
Copy link
Contributor Author

str4d commented Mar 5, 2024

Something else that the large binaries cause problems with: git pull. I have had my local checkout break multiple times, including just now:

$ git show HEAD
commit 887b214dcf3cc2ff5f609013328bb24933f6ac9c (HEAD -> main, ffi-0.6.0)
Merge: 7c19fd3 9a987c2
Author: Kris Nuttycombe <[email protected]>
Date:   Tue Jan 30 00:33:42 2024 -0700

    Merge pull request #112 from Electric-Coin-Company/ffi-0.5.0
    
    Feature branch for 0.5.0

$ git pull
Updating 887b214..f48bc78
error: unable to read sha1 file of releases/XCFramework/libzcashlc.xcframework/ios-arm64_x86_64-simulator/libzcashlc.framework/libzcashlc (b8e5131b6c306feaa63411eceac00722ea056816)
error: unable to read sha1 file of releases/XCFramework/libzcashlc.xcframework/macos-arm64_x86_64/libzcashlc.framework/libzcashlc (4815dfb60c24312ed6ba0c8d1818d974ce549855)
error: unable to read sha1 file of rust/Cargo.lock (0aa7a3a5600c9ac9158180cb0fec72e5673dc385)
error: unable to read sha1 file of rust/Cargo.toml (b5f9621002eca6baaaf945184842c0521981c9d8)
error: unable to read sha1 file of rust/src/lib.rs (97a2af3bf6c27d68ac8b724f28b40f9506c3d74c)

This makes development of the Rust FFI untenable: I cannot be wiping away my local repo every time a new release occurs, destroying all of my local stashed changes and in-progress branches.

@pacu
Copy link
Contributor

pacu commented Mar 7, 2024

so if we do want to enable ZcashLightClientKit to be used as a general Swift library

I don't think there is much sense to do this at the moment since the use case would be that a server-side Swift application actually uses it. A Linux user probably has a much better developer experience with the Kotlin SDK than with the swift one because of the nature of how Swift for Linux is supported.

Also, I created an issue for a matter that will be discussed tomorrow about better supporting React native wallets by bringing cocoapods back. Electric-Coin-Company/zcash-swift-wallet-sdk#1386.

This specific workflow

If we keep the Package.swift in this repository then there is a tagging dance that needs to happen, where we would need to:

  • Build the binary artifacts.
  • Update Package.swift to reference them, including checksum.
  • Tag the release.
  • Upload the binary artifacts to the release.

along with the cocoapods one could be automated to be lessen of a burden to the development team.

@pacu
Copy link
Contributor

pacu commented Mar 7, 2024

Some literature about this problem we are having that could be useful to us (and mostly EDGE)
https://hackernoon.com/cocoapod-as-xcframework-with-dependencies

@pacu
Copy link
Contributor

pacu commented May 21, 2024

This might be of interest for @str4d and @nuttycom

I'm working on the frost-swift-sdk PoC , which is a swift + uniffi thing. There are myriad of problems around the release of stuff, because I don't know how to combine a monorepo and github releases. people from Eiger actually created a separate repo for the Swift code and did a bunch of CI magic with gihub actions to push the swift code from their uniffi actions to their swift flavor demo. pretty cool, but also awful and unsustainable.

Anyhow I found a couple of things:

https://github.com/antoniusnaumann/cargo-swift/ which is a cargo plugin that works with uniffi to create a swift package out of it.
Then this github action that actually can be useful for the zcash ffi repo https://github.com/marketplace/actions/swift-create-xcframework it creates an xcframework and leaves everything ready for releasing through github releases instead of commiting the binary into the repo.
Unfortunately the github action is unmaintained and there's a fork for it.... classic FOSS

https://github.com/segment-integrations/swift-create-xcframework

@str4d
Copy link
Contributor Author

str4d commented Nov 20, 2024

I've now seen evidence of my issue with repo corruption (#96 (comment)) occurring in a downstream developer's Xcode while they were attempting to build Zashi iOS.

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

No branches or pull requests

2 participants