Add parse-bridges CLI subcommand #274

integer-overflown · 2024-05-10T22:25:50Z

What's this?

A new parse-bridges subcommand.
Also, an update to the documentation in the example showing this feature's preview.

This is handy for usage in higher-level build systems.
For example, in my case, I build both Rust and Swift with CMake, and having this CLI command implemented would allow generating the glue code from CMake as well, as a dependency step before building Rust/Swift targets (via add_custom_target API).

Notes

One caveat of the current implementation is that one would have to duplicate the crate name in the invocation line (the first argument).

This is fine for cases like mine, where this would be taken from the build system anyway, but it may not be handy for other cases.

The package name can be automatically deduced if one's running in the correct directory (the package root).
In this case, we can parse the cargo read-manifest output and get the name from there.

This would require a new dependency, though (serde_json), so I decided not to do this just yet, but if this sounds okay to you, I'll go ahead and implement this as well.

Examples

Single file:

swift-bridge-cli parse-bridges --crate-name my-awesome-crate -f src/lib.rs -o generated

Multiple files:

swift-bridge-cli parse-bridges --crate-name my-superb-crate \
-f src/lib.rs \
-f src/some_other_file.rs \
-o generated

chinedufn

Thanks for putting this together. Left some minor feedback.

One caveat of the current implementation is that one would have to duplicate the crate name in the invocation line (the first argument).

I'm not understanding what you mean here.

but if this sounds okay to you

Currently no known use cases where people would be repeatedly typing out this command by hand.

So, let's hold off on simplifying it and prefer explicit/obvious behavior for now.

Please update your PR body with an example of how the command looks.

When writing release notes we go to the PRs to grab examples.

We'll also sometimes link someone to an old similar PR, so it's nice to be able to look at the PR and immediately see what it's accomplishing.

chinedufn · 2024-06-01T22:19:34Z

crates/swift-bridge-cli/src/clap_exec.rs

+    let output = matches.get_one::<String>("output")
+        .map(Path::new)
+        .unwrap_or_else(|| Path::new(DEFAULT_OUTPUT_DIR));


Let's remove the default and require an explicit output directory.

Then we can consider introducing a default in the future if we decide that needing to specify the directory yourself is too cumbersome.

Reasons:

One fewer thing to figure out during this review. Adding a default would be a non-breaking change.

Fewer defaults helps teach the user about how things work / what is configurable

It is likely that users will write this command once and then re-use it, so it's okay if it's a little longer

Good point, done.

I've force-pushed the changes.

chinedufn · 2024-06-01T22:22:40Z

book/src/building/swiftc-and-cargo/README.md

-# The swift-bridge CLI does not exist yet. Open an issue if you need to use
-# this approach and I'll happily whip up the CLI.
-swift-bridge -f src/lib.rs > generated
+swift-bridge-cli parse-bridges <package name> src/lib.rs


Let's use --file / -f for each source file:

Let's use --crate-name for the crate name:

Reasons:

allow arguments to be specified in any order

more clear which argument is which. Right now <package name> and <file name> are right next to each other without any flags, making it a little harder to understand what each argument does.

Done; updated the docs as well

chinedufn · 2024-06-01T22:25:26Z

crates/swift-bridge-cli/src/clap_app.rs

+                .short('o')
+                .takes_value(true)
+                .value_name("PATH")
+                .help("Output destination folder; subfolders are created automatically")


No need to mention that subfolders are created automatically.
This information isn't useful to the user.

I found this helpful :)

But I might be in the minority here, so I removed this.

integer-overflown · 2024-06-03T20:38:01Z

Thanks for reviewing.

I'm not understanding what you mean here.

Inferring crate name from the manifest if a user is located in the package root.

Currently no known use cases where people would be repeatedly typing out this command by hand.

So, let's hold off on simplifying it and prefer explicit/obvious behavior for now.

I agree; this would be less error-prone and more explicit.
As you also mentioned, this command will likely be written once, and then integrated into a script or build-system rules.

Please update your PR body with an example of how the command looks.

Good point; done, see the examples section.

integer-overflown · 2024-06-03T20:41:49Z

book/src/building/swiftc-and-cargo/README.md

-    let out_dir = PathBuf::from("./generated");
-
-    let bridges = vec!["src/lib.rs"];
-    for path in &bridges {
-        println!("cargo:rerun-if-changed={}", path);
-    }
-
-    swift_bridge_build::parse_bridges(bridges)
-        .write_all_concatenated(out_dir, env!("CARGO_PKG_NAME"));


I removed this, as this is going to be handled by the shell script and the CLI.

Looks like the file now includes an example of generating the headers via API and via CLI, so this sounds good.

integer-overflown · 2024-06-03T20:45:03Z

book/src/building/swiftc-and-cargo/README.md

-
-    swift_bridge_build::parse_bridges(bridges)
-        .write_all_concatenated(out_dir, env!("CARGO_PKG_NAME"));
-
    println!("cargo:rustc-link-lib=static=swiftc_link_rust");


Also, something I noticed when modifying the docs...

This is the first and the last time "swiftc_link_rust" appears in the file.

This may be a typo.

Based on the swiftc command below, I think it will produce a "my_swift.a" file (based on the module name), so this should be

Suggested change

println!("cargo:rustc-link-lib=static=swiftc_link_rust");

println!("cargo:rustc-link-lib=static=my_swift");

What do you think?

Yup nice catch. You can make this change then I'll land this PR.

I see that there is a Commit suggestion button but I'm not sure if it would say that I committed it or that you committed it. I don't want to take credit for your work.

Thanks for noticing this.

chinedufn · 2024-06-13T09:35:03Z

book/src/building/swiftc-and-cargo/README.md

-    let out_dir = PathBuf::from("./generated");
-
-    let bridges = vec!["src/lib.rs"];
-    for path in &bridges {
-        println!("cargo:rerun-if-changed={}", path);
-    }
-
-    swift_bridge_build::parse_bridges(bridges)
-        .write_all_concatenated(out_dir, env!("CARGO_PKG_NAME"));


Looks like the file now includes an example of generating the headers via API and via CLI, so this sounds good.

chinedufn · 2024-06-13T09:39:52Z

Thanks for putting this together. Can land once you:

fix the my_swift.a that you noticed
get tests passing (looks like you need to run cargo fmt)

integer-overflown · 2024-06-14T16:27:37Z

@chinedufn done, pushed the changes :)

This produced additional warnings in stderr, which added unexpected output and caused a mismatch inside a test and thus panicked.

integer-overflown · 2024-06-15T20:58:11Z

@chinedufn I've pushed another commit that fixes the tests.

At least, on my side, cargo test --all passes successfully.

I'm not sure how my changes broke the UI tests.
There was probably some update on the CI runner side, which made the compiler more verbose and caused the test to fail.

I've put an explainer in the commit message.

Hopefully, it passes now 😅

NiwakaDev · 2024-06-15T22:45:19Z

I'm not sure how my changes broke the UI tests.

@integer-overflown

You need to address #276 (comment).

chinedufn · 2024-06-16T01:33:53Z

Congrats on landing your first contribution. Thank you for pushing this to completion. Good stuff.

integer-overflown force-pushed the add-parse-bridges-cli-subcommand branch from c888422 to 348ddb4 Compare May 11, 2024 22:03

integer-overflown mentioned this pull request May 11, 2024

How can I get the CLI? #106

Open

chinedufn reviewed Jun 1, 2024

View reviewed changes

integer-overflown force-pushed the add-parse-bridges-cli-subcommand branch from 348ddb4 to 3f78c00 Compare June 3, 2024 20:30

integer-overflown commented Jun 3, 2024

View reviewed changes

integer-overflown requested a review from chinedufn June 3, 2024 20:45

chinedufn approved these changes Jun 13, 2024

View reviewed changes

integer-overflown force-pushed the add-parse-bridges-cli-subcommand branch from 3f78c00 to 86a8d64 Compare June 14, 2024 11:58

integer-overflown added 3 commits June 14, 2024 15:02

cli: add parse-bridges cli subcommand

3ef471c

book: update swiftc-and-cargo to use new subcommand

0ab35e2

book: correct Swift lib name

9dd787f

integer-overflown force-pushed the add-parse-bridges-cli-subcommand branch from 86a8d64 to 9dd787f Compare June 14, 2024 12:02

tests: ignore unused variables

960c5f3

This produced additional warnings in stderr, which added unexpected output and caused a mismatch inside a test and thus panicked.

chinedufn merged commit 717fcef into chinedufn:master Jun 16, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parse-bridges CLI subcommand #274

Add parse-bridges CLI subcommand #274

integer-overflown commented May 10, 2024 •

edited

Loading

chinedufn left a comment

chinedufn Jun 1, 2024

integer-overflown Jun 3, 2024

chinedufn Jun 1, 2024

integer-overflown Jun 3, 2024 •

edited

Loading

chinedufn Jun 1, 2024

integer-overflown Jun 3, 2024

integer-overflown commented Jun 3, 2024

integer-overflown Jun 3, 2024

chinedufn Jun 13, 2024

integer-overflown Jun 3, 2024

chinedufn Jun 13, 2024 •

edited

Loading

chinedufn Jun 13, 2024

chinedufn commented Jun 13, 2024

integer-overflown commented Jun 14, 2024

integer-overflown commented Jun 15, 2024

NiwakaDev commented Jun 15, 2024

chinedufn commented Jun 16, 2024

	println!("cargo:rustc-link-lib=static=swiftc_link_rust");
	println!("cargo:rustc-link-lib=static=my_swift");

Add parse-bridges CLI subcommand #274

Add parse-bridges CLI subcommand #274

Conversation

integer-overflown commented May 10, 2024 • edited Loading

What's this?

Notes

Examples

chinedufn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

integer-overflown Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

integer-overflown commented Jun 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinedufn Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinedufn commented Jun 13, 2024

integer-overflown commented Jun 14, 2024

integer-overflown commented Jun 15, 2024

NiwakaDev commented Jun 15, 2024

chinedufn commented Jun 16, 2024

integer-overflown commented May 10, 2024 •

edited

Loading

integer-overflown Jun 3, 2024 •

edited

Loading

chinedufn Jun 13, 2024 •

edited

Loading