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

Add parse-bridges CLI subcommand #274

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

integer-overflown
Copy link

@integer-overflown integer-overflown commented May 10, 2024

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

Copy link
Owner

@chinedufn chinedufn left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines 51 to 53
let output = matches.get_one::<String>("output")
.map(Path::new)
.unwrap_or_else(|| Path::new(DEFAULT_OUTPUT_DIR));
Copy link
Owner

Choose a reason for hiding this comment

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

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:

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

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

  3. 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

Copy link
Author

Choose a reason for hiding this comment

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

Good point, done.

I've force-pushed the changes.

# 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
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Done; updated the docs as well

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

I found this helpful :)

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

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

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.

Comment on lines -145 to -153
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"));
Copy link
Author

Choose a reason for hiding this comment

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

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


swift_bridge_build::parse_bridges(bridges)
.write_all_concatenated(out_dir, env!("CARGO_PKG_NAME"));

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

Choose a reason for hiding this comment

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

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?

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.

None yet

2 participants