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

Consider splitting wasm-opt crate into separate lib and bin crate #93

Open
dtolnay opened this issue Oct 13, 2022 · 4 comments
Open

Consider splitting wasm-opt crate into separate lib and bin crate #93

dtolnay opened this issue Oct 13, 2022 · 4 comments

Comments

@dtolnay
Copy link
Contributor

dtolnay commented Oct 13, 2022

I sent you this in response to your email @brson, but I realized afterward GitHub issues may be a saner medium to discuss the several unrelated topics I wanted to bring up.


The rationale would be:

  • Cargo has no ability to declare binary-only deps or library-only deps. In your current setup your wasm-opt binary depends on the wasm-opt library AND the wasm-opt-cxx-sys crate, neither of which it uses, so anyone installing it will be compiling those 2 things (and all their transitive deps, including cxx) needlessly. If the only reason to bundle both the lib and the bin under one crate name is the "vanity" aspect of "wasm-opt" being the ideal name for both of them, I recognize that but it wouldn't be compelling to me at the expense of the downsides.

  • Split crates allows what I think is a better solution regarding the "Calling the C++ main function from Rust" section. I would recommend deleting your wasm_opt_main try/catch shim from wasm-opt-sys and your wasm_opt_main pointer munging logic from wasm-opt's main.rs. Instead I would recommend turning "do you want THROW_ON_FATAL" into a Cargo feature on wasm-opt-sys. The lib would set it and the bin would not set it (or maybe the inverse feature would be better). Then in wasm-opt-sys build.rs, make it skip the string replacement of int main to wasm_opt_main_actual under the feature that the bin uses (i.e. keep int main intact), and change wasm-opt's main.rs to #![no_main]. In the absence of a Rust main, execution will jump directly into the C++ main without any Rust involved.

@dtolnay
Copy link
Contributor Author

dtolnay commented Oct 13, 2022

I did put both the lib and bin in the same crate for the sake of having the nice name in both situations. Are you aware of any established naming conventions for bin/lib split crates? Give the bin the desired name and the lib wasm-opt-lib; the opposite?

I am not aware of an established convention. Yours seems probably as reasonable as any. I think it depends whether you expect more use of the bin or the lib in practice to decide which one you'll give the better crate name. If you end up renaming the bin crate remember to add [[bin]] name = "wasm-opt" to make cargo install install the compiled binary under that name.

@brson
Copy link
Owner

brson commented Oct 15, 2022

This is a reasonable idea, but I'm going to sit on it for a bit. Needs a major version bump anyway.

The major drawback of the dual bin/lib crate from the end user perspective is that wasm-opt bin needs to build wasm-opt-cxx-sys but doesn't use it. The build time of wasm-opt-cxx-sys is not great compared to wasm-opt-sys though, so that doesn't bother me much.

Split crates making main handling more reliable is something I hadn't considered and would be a great simplification.

@brson
Copy link
Owner

brson commented Oct 15, 2022

Also of course the non-split crate forces a build of the wasm-opt lib to build the bin. It is also not a great deal of code. wasm-opt-sys dominates the build.

@dtolnay
Copy link
Contributor Author

dtolnay commented Oct 15, 2022

As described in #94 (comment), another reason in favor of splitting is you're going to want different versioning schemes for the lib vs the bin. Bin should be 0.110.0 matching the binaryen version, lib should be 0.1.0+binaryen.0.110.0 keeping track of Rust-facing breaking changes independent of binaryen. It seems extremely likely to me that you will have both binaryen updates which do not entail breaking the Rust API, and vice versa Rust-specific iteration on the API which does not involve binaryen getting updated.

You can't version separately if there is only one crates.io package containing both.

@brson brson added this to the M3 milestone Oct 15, 2022
@brson brson removed this from the M3 milestone Nov 11, 2022
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

No branches or pull requests

2 participants