Speed up building xcframework for local development #61

crazytonyli · 2024-04-09T05:17:02Z

At the moment, we run make xcframework when we update the rust code and need to use them in the Swift package. The command builds the rust library against all Apple platforms (11 targets in total). But in reality, most of the time, we really only need libraries for one platform or one target—the one we choose in Xcode.

Alternatives to the `make xcframework` command

This PR introduces a couple of new things:

make xcframework-only-<ios|macos|..> to create a xcframework for a given platform, which will save us from building rust library for other platforms that we don't need during development.
By default, use debug configuration (a.k.a cargo dev profile) during development and use release configuration on CI, which will also save us build time during development because dev build is about 1.2 times faster than release build.

New `tools` cargo binary package

This PR adds a new tools binary package to Cargo. It's a command line application, which for the moment only has a create-xcframework command. We may need to add more later (for example, release process).

$ cargo run --quiet --bin tools -- --help
Usage: tools <COMMAND>

Commands:
  create-xcframework
  help                Print this message or the help of the given subcommand(s)

Options:
  -h, --help  Print help

$ cargo run --quiet --bin tools -- create-xcframework --help
Usage: tools create-xcframework [OPTIONS] --targets <TARGETS>...

Options:
      --targets <TARGETS>...  List of targets whose static libraries should be included in the xcframework
      --profile <PROFILE>     Cargo profile used to build the targets [default: release]
  -h, --help                  Print help

The create-xcframework command is a more convenient way to call xcodebuild -create-xcframework. It takes a list of cargo targets(which should have been built by this point), combine some of them into fat libraries if needed, and then call the xcodebuild command to create a xcframework. That enables us easily creating xcframework for selected targets/platforms from Makefile.

By using this tool, Makefile now only builds targets and the tools create-xcframework command takes care of creating an xcframework from built targets.

Test Instructions

The existing make xcframework should be covered by CI.

You can run [CARGO_PROFILE=dev|release] make clean xcframework-only-<platform> locally to verify xcframework is built accordingly.

You can also try calling the create-xcframework tool directly, but you'll need to build some targets before passing them to command line.

cargo run --quiet --bin tools -- create-xcframework --targets ...

crazytonyli · 2024-04-09T05:18:34Z

Makefile

+xcframework-only-macos: $(build-apple-platform-macos)
+xcframework-only-ios: $(build-apple-platform-ios)
+xcframework-only-tvos: $(build-apple-platform-tvos)
+xcframework-only-watchos: $(build-apple-platform-watchos)


I tried to consolidate all of the above into one wildcard target: xcframework-only-%, but couldn't get it to work. Let me know if you have any ideas.

oguzkocer

@crazytonyli I went through the Rust implementation and left you some non-blocking notes. In some cases, I stopped duplicating some of the comments as the earlier ones applied elsewhere as well. I can mention them in my follow up review if necessary.

oguzkocer · 2024-05-28T14:54:43Z

tools/src/xcframework.rs

+
+use crate::Action;
+
+static LIBRARY_FILENAME: &str = "libwordpress.a";


Suggested change

static LIBRARY_FILENAME: &str = "libwordpress.a";

const LIBRARY_FILENAME: &str = "libwordpress.a";

I assume this value is not supposed to be modified with a 'static lifetime? If so, we should use const instead.

oguzkocer · 2024-05-28T14:57:35Z

tools/src/xcframework.rs

+    std::fs::create_dir_all(dir)
+        .with_context(|| format!("Failed to create directory: {:?}", dir))?;
+
+    Ok(())


Suggested change

std::fs::create_dir_all(dir)

.with_context(|| format!("Failed to create directory: {:?}", dir))?;

Ok(())

std::fs::create_dir_all(dir).with_context(|| format!("Failed to create directory: {:?}", dir))

You can return the result directly since create_dir_all returns std.io::Result<()>.

oguzkocer · 2024-05-28T15:01:57Z

tools/src/xcframework.rs

+
+impl XCFramework {
+    fn new(targets: &Vec<String>, profile: &str) -> Result<Self> {
+        let headers = PathBuf::from("target/swift-bindings/headers");


Should "target/swift-bindings/headers" be a const?

oguzkocer · 2024-05-28T15:03:54Z

tools/src/xcframework.rs

+            return Err(Error::msg(format!(
+                "Headers not found: {}",
+                headers.display()
+            )));


Suggested change

return Err(Error::msg(format!(

"Headers not found: {}",

headers.display()

)));

anyhow::bail!("Headers not found: {}", headers.display());

oguzkocer · 2024-05-28T15:06:03Z

tools/src/xcframework.rs

+        let mut parts = target.split('-');
+        _ /* arch */= parts.next();
+        if parts.next() != Some("apple") {
+            return Err(Error::msg(format!("{} is not an Apple platform", target)));


Suggested change

return Err(Error::msg(format!("{} is not an Apple platform", target)));

anyhow::bail!("{} is not an Apple platform", target);

oguzkocer · 2024-05-28T15:09:12Z

tools/src/xcframework.rs

+        let os = match parts.next() {
+            Some("darwin") => ApplePlatform::MacOS,
+            Some("ios") => ApplePlatform::IOS,
+            Some("tvos") => ApplePlatform::TvOS,
+            Some("watchos") => ApplePlatform::WatchOS,


This is probably worth implementing as ApplePlatform::from_str.

oguzkocer · 2024-05-28T15:12:49Z

tools/src/xcframework.rs

+        let llvm_target = json
+            .get("llvm-target")
+            .and_then(|t| t.as_str())
+            .ok_or(Error::msg("No llvm-target in command output"))?;


Should we use with_context for consistency?

Suggested change

.ok_or(Error::msg("No llvm-target in command output"))?;

.with_context(|| format!("No llvm-target in command output"))?;

oguzkocer · 2024-05-28T16:18:55Z

tools/src/xcframework.rs

+        let mut libraries: Vec<PathBuf> = Vec::new();
+        for lib in &self.libraries {
+            libraries.push(lib.create(temp_dir)?);
+        }
+        Ok(libraries)


Suggested change

let mut libraries: Vec<PathBuf> = Vec::new();

for lib in &self.libraries {

libraries.push(lib.create(temp_dir)?);

}

Ok(libraries)

self.libraries

.iter()

.map(|lib| lib.create(temp_dir))

.collect()

oguzkocer · 2024-05-28T16:20:44Z

tools/src/xcframework.rs

+
+impl Action for CreateXCFramework {
+    fn run(&self) -> Result<()> {
+        let temp_dir = std::env::temp_dir().join("wp-rs-xcframework");


Should wp-rs-xcframework be a const?

I'll leave this string local to the function, because it's a temporary directory name which can change without affecting anyone.

oguzkocer · 2024-05-28T16:23:28Z

tools/src/xcframework.rs

+        recreate_directory(&dest)?;
+        std::fs::rename(temp_dest, &dest).with_context(|| "Failed to move xcframework")?;
+
+        println!("xcframework created at {}", &dest.display());


Suggested change

println!("xcframework created at {}", &dest.display());

println!("xcframework created at {}", dest.display());

jkmassel

Chatted about whether we want to use this or a different scripting language for this (ruby, because it's our release tooling language, or Swift because this tool is for folks writing swift code)

We'll land it as-is for now (with Oguz's comments addressed), and we can adjust later if we need to.

Thanks Tony!

crazytonyli · 2024-05-28T23:41:44Z

@oguzkocer Thanks for your review. I have addressed all your comments. I'll merge this PR after CI jobs pass. Happy to address any further comments in follow up PRs.

crazytonyli added 2 commits April 9, 2024 17:13

Add a command line tool to create xcframework

fbb047e

Update Makefile to use the new create-xcframework tool

e416aed

crazytonyli requested review from jkmassel and oguzkocer April 9, 2024 05:17

crazytonyli commented Apr 9, 2024

View reviewed changes

crazytonyli added 3 commits April 9, 2024 17:20

Remove tabs

0ca2401

Merge branch 'trunk' into create-xcframework-in-rust

9e874ec

Merge branch 'trunk' into create-xcframework-in-rust

4b0a323

crazytonyli mentioned this pull request May 1, 2024

Build for watchOS real devices #71

Merged

crazytonyli added 2 commits May 3, 2024 09:18

Build for watchOS real devices (#71)

28212df

Merge branch 'trunk' into create-xcframework-in-rust

ab82a91

crazytonyli mentioned this pull request May 15, 2024

Add configurations to include full debug info in static libraries #59

Merged

crazytonyli added 3 commits May 16, 2024 13:30

Merge branch 'trunk' into create-xcframework-in-rust

03c4c32

Merge branch 'trunk' into create-xcframework-in-rust

2101394

Merge branch 'trunk' into create-xcframework-in-rust

b567ae8

oguzkocer reviewed May 28, 2024

View reviewed changes

jkmassel approved these changes May 28, 2024

View reviewed changes

crazytonyli added 3 commits May 29, 2024 11:23

Address PR comments

1342520

Rename tools to xcframework

4c28720

Merge branch 'trunk' into create-xcframework-in-rust

9f7430a

crazytonyli enabled auto-merge (squash) May 28, 2024 23:40

crazytonyli merged commit d320476 into trunk May 28, 2024

crazytonyli deleted the create-xcframework-in-rust branch May 28, 2024 23:50

crazytonyli mentioned this pull request May 29, 2024

Support watchOS device #48

Closed


		use crate::Action;

		static LIBRARY_FILENAME: &str = "libwordpress.a";

	static LIBRARY_FILENAME: &str = "libwordpress.a";
	const LIBRARY_FILENAME: &str = "libwordpress.a";

	return Err(Error::msg(format!("{} is not an Apple platform", target)));
	anyhow::bail!("{} is not an Apple platform", target);

	.ok_or(Error::msg("No llvm-target in command output"))?;
	.with_context(\|\| format!("No llvm-target in command output"))?;

	println!("xcframework created at {}", &dest.display());
	println!("xcframework created at {}", dest.display());

Speed up building xcframework for local development #61

Speed up building xcframework for local development #61

Uh oh!

Conversation

crazytonyli commented Apr 9, 2024

Alternatives to the make xcframework command

New tools cargo binary package

Test Instructions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oguzkocer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkmassel left a comment

Choose a reason for hiding this comment

Uh oh!

crazytonyli commented May 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Alternatives to the `make xcframework` command

New `tools` cargo binary package