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

Rust build plugin #1443

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Rust build plugin #1443

wants to merge 2 commits into from

Conversation

str4d
Copy link
Collaborator

@str4d str4d commented Jun 5, 2024

This is an experiment to try and recreate the CocoaPods build scripts (that were removed in 86d1257) as a Swift Package Manager build plugin. If we can do so, then we can use that for regular development, and swap it out with a binaryTarget in release PRs (the tagged release commits would use the binary targets, and then the commit immediately afterwards reverts to using the build plugin).

To test this draft PR, create a symlink in the root directory named rust, pointing to the rust folder in a local checkout of zcash-light-client-ffi at the correct tag. Once we get the build plugin fully working, I will rework this PR to import the Rust code from zcash-light-client-ffi (after which we can archive that repository).

Part of Electric-Coin-Company/zcash-light-client-ffi#96.


This code review checklist is intended to serve as a starting point for the author and reviewer, although it may not be appropriate for all types of changes (e.g. fixing a spelling typo in documentation). For more in-depth discussion of how we think about code review, please see Code Review Guidelines.

Author

  • Self-review: Did you review your own code in GitHub's web interface? Code often looks different when reviewing the diff in a browser, making it easier to spot potential bugs.
  • Automated tests: Did you add appropriate automated tests for any code changes?
  • Code coverage: Did you check the code coverage report for the automated tests? While we are not looking for perfect coverage, the tool can point out potential cases that have been missed.
  • Documentation: Did you update Docs as appropiate? (E.g README.md, etc.)
  • Run the app: Did you run the app and try the changes?
  • Did you provide Screenshots of what the App looks like before and after your changes as part of the description of this PR? (only applicable to UI Changes)
  • Rebase and squash: Did you pull in the latest changes from the main branch and squash your commits before assigning a reviewer? Having your code up to date and squashed will make it easier for others to review. Use best judgement when squashing commits, as some changes (such as refactoring) might be easier to review as a separate commit.

Reviewer

  • Checklist review: Did you go through the code with the Code Review Guidelines checklist?
  • Ad hoc review: Did you perform an ad hoc review? In addition to a first pass using the code review guidelines, do a second pass using your best judgement and experience which may identify additional questions or comments. Research shows that code review is most effective when done in multiple passes, where reviewers look for different things through each pass.
  • Automated tests: Did you review the automated tests?
  • Manual tests: Did you review the manual tests?You will find manual testing guidelines under our manual testing section
  • How is Code Coverage affected by this PR? We encourage you to compare coverage befor and after your changes and when possible, leave it in a better place. Learn More...
  • Documentation: Did you review Docs, README.md, LICENSE.md, and Architecture.md as appropriate?
  • Run the app: Did you run the app and try the changes? While the CI server runs the app to look for build failures or crashes, humans running the app are more likely to notice unexpected log messages, UI inconsistencies, or bad output data.

@pacu
Copy link
Contributor

pacu commented Jun 5, 2024

@y4ssi and I have been discussing this approach privately. I'd like to share some thoughts I had

We originally moved out of the "build system" approach because of (swift) developer friction points:

  1. Tooling at the time failed a lot.
  2. made Swift developers download the rust environment to contribute or build locally.
  3. Builds were not reproducible which complicated debugging production issues.
  4. builds (that didn't fail) were slow.

the second point is a "philosophy" thing more than anything. Would you mandate Swift developers to download Rust, cargo and rustup to their environment just for them to contribute a pure Swift change your project?

I believe it was unreasonable from my part as a maintainer. I'm not a maintainer of this repo anymore so, you decide 😅.

The rest are "concrete" concerns.

Build reproducibility
Given that Tags can be made so that they point to a binary artifact, production build reproducibility is somewhat guaranteed.

The problem is mostly Local reproducibility, because of the RustLang environment every developer has.

I don't know how you folks are deploying Zashi to Testflight, but if you were to deploy manually from source, then Zashi would build the SDK, the SDK would build Rust via a Plugin, and that would be then archived and uploaded to testflight.

Is there a way that we can set up the environment so that every developer working on the SDK has the same cargo and rust toolchain? Is that possible to enforce to CI runners as well?

I recall this problem was solved in Ruby by a tool call Bundler. It was used in iOS and Mac development to make reproducible CocoaPods builds as well. Is there something like that for Rust?

Build Reliability
At the time (Sept 2019) there were no Swift Package Plugins. So a build system was created to do all the Rust heavy-lifting using cocopods pre and post build hooks. Today, thanks to SPM plugins existing and being mature enough this could be done another way so that the builds are more stable and reliable. Although it has to be noted that swiftpm and Xcode are caching beasts and weirdness is assured

Build times

adding the rust tooling will definitely increase CI build times. I guess that the assessment is that the time spent waiting on builds will be fairly compensated by the time spent working on two different repos.

Future-proof approach
The cocoapods build system was not even "present proof" 😅 but it was what worked at the time.
I think that the zcash-swift-wallet-sdk --> zcash-light-client-ffi approach is future-proof in terms of
allowing the FFI to move to UniFFI, diplomat or other kind of FFI handling solution. This will be an investment
towards continuing to bake the FFI cake ourselves, rather than going to a unified approach were RustLang FFI
changes are made once without needing Rust developers to work on the FFI of Kotlin and Swift.

Two problems that this PR seems to be trying to address

  1. Repo bloat because of the FFI artifacts
  2. Rust development friction

Problem 1 can be addressed with some approach like the one explored there

Problem 2 is more complex. I think that some questions are worth to be asked and answered:

  • Should Rust developers be "full-stacking" their way over kotlin-rust and swift-rust?
  • What's the % of work done on purely on Swift versus the one done in Rust?

Overall comments

I think its worth exploring. But a unified FFI approach with better test surface would be the best.

If we can do so, then we can use that for regular development, and swap it out with a binaryTarget in release PRs (the tagged release commits would use the binary targets, and then the commit immediately afterwards reverts to using the build plugin).

This can possibly be achieved by using conditional compilation within the Package.swift file so that there's an environment variable on the release CI job that is passed to the build, that will make the compiler use the plugin or just compile using a .binaryTarget with an URL and a checksum.

@str4d
Copy link
Collaborator Author

str4d commented Jun 10, 2024

Would you mandate Swift developers to download Rust, cargo and rustup to their environment just for them to contribute a pure Swift change your project?

Yes, because this is not a pure Swift project; it is a hybrid Swift/Rust project. (And they don't even need to do that if they don't build locally and instead rely on CI to check their changes.)

As a Rust developer, I will never have the luxury of not using Xcode (I wish I did) or Swift to contribute a pure Rust change if I want to test that it works. Moreover, the majority of functionality changes in this project require Rust changes, and the current process is so hostile to Rust development that I have on multiple occasions considered quitting entirely. Why should we expect contributors to be more willing to contribute?

Not requiring downstream users of the SDK to install Rust is a much more reasonable goal.

Is there a way that we can set up the environment so that every developer working on the SDK has the same cargo and rust toolchain? Is that possible to enforce to CI runners as well?

Yes, and we already use it. The FFI repo has a rust-toolchain.toml file that tells rustup what toolchain to use by default; that would be migrated to here in this PR once we get the build system working. Any CI that uses rustup (e.g. GitHub Actions, but we can install it on others) will pick it up automatically.

If you aren't using rustup then you might be using a different Rust version, but you cannot use a Rust version older than the MSRV specified in the Cargo.toml, and we bump that regularly to basically track stable Rust. So developers with a too-old Rust version will notice immediately.

adding the rust tooling will definitely increase CI build times. I guess that the assessment is that the time spent waiting on builds will be fairly compensated by the time spent working on two different repos.

There's a bunch of caching we can apply to CI to improve Rust build times (that we already apply in our other repos to great effect). It will be slightly finicky due to building multiple targets at once, but that can be handled.

Moreover, I think for local development it should be possible to only build the target you are deploying to (the simulator, or your local iOS device). That would greatly speed up local recompilations.

allowing the FFI to move to UniFFI, diplomat or other kind of FFI handling solution. This will be an investment
towards continuing to bake the FFI cake ourselves, rather than going to a unified approach were RustLang FFI changes are made once without needing Rust developers to work on the FFI of Kotlin and Swift.

Even in the grand future of UniFFI or Diplomat unifying the Rust backends of the Swift and Kotlin SDKs (which I do want to move towards), we still have the issue of enabling feature branches where the SDKs have partially-implemented features with potentially-breaking changes still to come. Developers need to be able to use any revision of the Rust backend, without requiring it to be in a state suitable for public consumption and subject to backwards compatibility requirements. This requires either that we build and cache binaries for every single commit (including across force-pushes to branches, and across arbitrary forks) that ever occurs on the Rust backend (to ensure that the pure Swift developers can reliably compile no matter what code they checked out), or we need to use local developer compilation. We do not have the devops resources for the former, and my proposal here is the latter.

@pacu
Copy link
Contributor

pacu commented Jun 10, 2024

As a Rust developer, I will never have the luxury of not using Xcode (I wish I did)

As a Swift developer, I will never have the luxury of not using Xcode (I wish I did) 😂

Thanks for the comments @str4d !

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