Skip to content

Conversation

jkmassel
Copy link
Contributor

Makes it possible to build the framework and package it as an XCFramework for use on Apple platforms.

Next steps deliberately not included:

  • Sample app(s)?
  • Automated publishing

@jkmassel jkmassel requested a review from oguzkocer November 22, 2023 17:44
Copy link
Contributor

@oguzkocer oguzkocer left a comment

Choose a reason for hiding this comment

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

@jkmassel This looks great, thank you for helping me out with it! 🙇‍♂️

I think we can merge this as is, but I have some suggestions as a follow up. I am happy to work on these myself, but I wanted to get your opinion before I make changes to it.

Also, it's worth noting that most of the rest of the project is not in a production level state. Especially the make tasks for Android are kind of all over the place at the moment, because I mostly used them as a way to speed up my initial work rather than something that should be used in the long run. I'll go back to them at some point and fix them up, but it's not a priority at the moment.

Cargo.toml Outdated
Comment on lines 8 to 9
[profile.release]
debug = true
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should be overriding the default profile. It'd be good to treat the Rust project as a standalone crate and in such a crate we wouldn't want to include the debug symbols in a release build.

I think the following 2 options would be better:

  1. We could use RUSTFLAGS environment variable which is what we do for Gradle builds
  2. We can add a custom profile and then customize it however we want

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'd prefer to avoid #2, because then it's easy to forget? #1 seems promising!

in such a crate we wouldn't want to include the debug symbols in a release build

This is something I don't have a deep understanding of, but here's why I figured it wasn't a huge deal – AFAICT there are two relevant cases to consider:

  1. If you're building the library standalone – if that's the case, you probably want to embed it in something else, so producing debug symbols is helpful (because maybe you want them over there?)
  2. If you're building the library as a dependency of a larger Rust project – I assume in that case that the compiler would ignore this directive and instead use the debug setting of the top-level Cargo.toml to decide what to do about debug symbols

LMK if I went wrong somewhere though!!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd prefer to avoid #2, because then it's easy to forget?

Could you elaborate on what would be easy to forget? I think it's a decent option if we want to heavily customize the builds for mobile consumption.

If you're building the library standalone – if that's the case, you probably want to embed it in something else, so producing debug symbols is helpful (because maybe you want them over there?)

But that's at best a maybe. Not enough to go against the convention in my opinion. Even in our project, I think we're actually not going to package the debug symbols for our Android release dependencies. With multiple architectures, even a small library adds up to 20+ MB with debug symbols whereas the release version would be around 1MB in total. 20 MB may not seem like much, but consider the case where each dependency did that 😱

If you're building the library as a dependency of a larger Rust project – I assume in that case that the compiler would ignore this directive and instead use the debug setting of the top-level Cargo.toml to decide what to do about debug symbols

I think that's correct. However, currently this option is added to the root level Cargo.toml file, so it effects every single crate within the project. For example, if we have a command line binary or a web crate, we'd also be shipping them with debug symbols. That's probably not we want, but even if it is, that should be deliberate decision.


In my opinion, adding debug symbols to the release builds is going against the convention and if we are going to do that, we should have a very good reason to do so. In this case, there are very simple alternatives, so I don't think there is enough justification for it. That's especially true if we don't package the debug symbols for our Android dependency.

That's my 2 cents, but this decision is not that important at the moment and we can always re-visit it. So, if you have a strong preference, let's go with that!

Makefile Outdated
cargo run --release --bin uniffi_bindgen generate --library ./target/release/libwordpress_api.dylib --out-dir ./target/swift-bindings --language swift
cp target/swift-bindings/wordpress_api.swift native/swift/Sources/wordpress-api-wrapper/wordpress_api.swift
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Is there a reason we are doing this in 2 separate steps instead of directly outputting to the folder we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly just for ease of debugging later

Makefile Outdated
Comment on lines 44 to 48
# Ensure we have the toolchains we need
rustup target add x86_64-apple-ios
rustup target add aarch64-apple-ios
rustup target add aarch64-apple-darwin
rustup target add x86_64-apple-darwin
rustup target add aarch64-apple-ios-sim
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not a fan of installing components as part of a build task - not unless there is a prompt to get out of it. I think we should have a separate make task to check if the necessary components are installed and offer to install them if they are missing. Then, we can depend on that task from this one.

This same comment applies to the nightly toolchain installation and adding the rust-src component below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, and I'm happy to break out prerequisites from the task. I was going for "it just works" here

Makefile Outdated
cp target/swift-bindings/wordpress_api.swift native/swift/Sources/wordpress-api-wrapper/wordpress_api.swift

# Most of this is extracted from `cargo-swift` and customized for our needs
generate-xcframework: generate-swift-bindings
Copy link
Contributor

Choose a reason for hiding this comment

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

This task is a bit massive at the moment. I think we can split it up to 3 tasks:

  1. That builds for all Apple architectures
  2. That combines the binaries using lipo
  3. That generates xcframework

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a very large task for sure! I'm just wondering if there's any benefit to doing any of the above on their own (or do any of them from other steps)? If not, my default would be to leave them combined until that point, but I don't feel super strongly about it

Copy link
Contributor

Choose a reason for hiding this comment

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

I think for now, the main benefit is that it would make it easier to understand what's happening in the task - especially for anyone who is unfamiliar with the iOS stack. Having tasks with meaningful names would make it self-documenting.

There are also cases where having separate steps would make it easier to debug the process.

I don't feel strong enough to push for it, but in general, having smaller tasks make sense to me.

@jkmassel jkmassel force-pushed the add/spm branch 3 times, most recently from ee72574 to 10fe3a2 Compare December 6, 2023 04:52
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