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

Restructure the CLI #702

Open
4 of 8 tasks
saulecabrera opened this issue Jul 11, 2024 · 10 comments
Open
4 of 8 tasks

Restructure the CLI #702

saulecabrera opened this issue Jul 11, 2024 · 10 comments
Assignees
Labels
enhancement New feature or request

Comments

@saulecabrera
Copy link
Member

saulecabrera commented Jul 11, 2024

This proposal suggests a restructuring of Javy's CLI interface to:

  • Enhance flexibility in specifying code generation options and JavaScript engine features.
  • Provide semantically meaningful command and sub-command names where applicable.

Command Flexibility

Javy currently offers a wide range of configuration options that control the JavaScript language features available to the generated code. However, there are limitations:

  • There is currently no way to specify these options directly from the CLI.
  • Other code-generation options, such as name section generation and optimization options, cannot be specified through the CLI. These options are crucial for profiling Javy-generated modules using tools like wasmprof. Currently, users must manually update these options, which is not ideal.
  • The current default settings are not well-suited to all use cases, as highlighted in issue #628.

Semantically Meaningful Commands, Sub-Commands and Options

The CLI currently features two main commands: compile and emit-provider. The compile command does not accurately reflect its actions and could be better named to represent actual AOT compilation of JavaScript, a feature we plan to explore in the future.

Proposal

The high-level proposal includes:

  • Deprecating the compile command in favor of a new command, tentatively named build. Avoid introducing breaking changes or new features through the compile command.
  • Enhance the build command with meaningful options to control JavaScript and code generation features as described above.
  • Updating the compile command to emit a deprecation notice, indicating that it will be deprecated in the next major release of the CLI.
    • The ideal timeline for transitioning from issuing a deprecation notice to actual deprecation is not clear. I propose to discuss this issue on our Zulip stream and make a decision based on the feedback received.

CLI Interface

The proposal suggests, according to the bullet points above, to have the new build command look like:

$ javy build --help

Builds a Wasm module from a JS source
Usage: javy build <OPTIONS> <JS>

Arguments:
  <JS> The JavaScript source to be used as input for building the WebAssembly module.

Options:

-J, --js <KEY[=VAL[,..]]> JavaScript runtime options.
-D, --debug <KEY[=VAL[,..]]> Debug options.
-C, --codegen <KEY[=VAL[,..]]> Code generation options (the current `-d` and `wit` options)
-o, --output The path of the Wasm module.

For example, in order to control the name section generation, one could do:

javy build -D generate-name-section=true -o index.wasm index.js

The suggested CLI layout, takes inspiration from Wasmtime's CLI layout, which, I think offers multiple benefits, notably:

  • Ability to have option groups, that make it easier to share such groups across commands where applicable (e.g., it might make sense to share the -D group, across the build and emit-provider commands, for cases in which the user wants to control walrus/wasm-opt options.
  • Reduced option verbosity. I can think of at least one alternative way of presenting command options, by using enable-<group>-<option> and disable-<group>-<option>. Aside from the verbosity, with this approach, we need to introduce at least two options per group (assuming each option allows at least enabling or disabling).

Proposed order of operations

  • Introduce the new build command, which will initially behave similar to the current compile command.
  • Introduce a deprecation notice under the current compile command.
  • Land CLI: Allow Input from stdin and Output to stdout #701
  • Release version 3.1.0 of the CLI
  • Introduce the -J options group
  • Introduce the -D options group
  • Introduce the -C options group
  • Release version 4 of the CLI with the deprecated compile command. (After having collected feedback / agreed on the deprecation timeline)
@saulecabrera saulecabrera added the enhancement New feature or request label Jul 11, 2024
@saulecabrera saulecabrera self-assigned this Jul 15, 2024
@jeffcharles
Copy link
Collaborator

A few questions to start:

  1. Where does the current -d flag fit in on the new interface?
  2. Is it fair to say the difference between the -J and -D groups are the -D options have more to do with the content of the Wasm that's created and the -J group of options have to do with intrinsics that are made available?
  3. Is the idea that the -J option group will impact the provider that's produced when running emit-provider?

@saulecabrera
Copy link
Member Author

saulecabrera commented Jul 15, 2024

  1. Updated the OP; I'm currently thinking under a -C, --codegen group.
  2. I intend the -D group to control debug options around code generation. Alternatively, merging the -D options under the -C group could also work. For -J your interpretation is correct.
  3. It could, however, I consider that change to be out-of-scope for this proposal.

@saulecabrera
Copy link
Member Author

It could, however, I consider that change to be out-of-scope for this proposal.

More explicitly for this proposal, I'm intending to validate that the option groups specified through -J are in agreement with the JS options enabled by default in the provider when -C dynamic=true is specified.

@jeffcharles
Copy link
Collaborator

1. Updated the OP; I'm currently thinking under a -C, --codegen group.

👍

2. I intend the -D group to control debug options around code generation. Alternatively, merging the -D options under the -C group could also work. For -J your interpretation is correct.

Merging them would help to avoid unproductive discussions about when a new codegen-related flag is actually a debug flag or the opposite.

3. [...] I'm intending to validate that the option groups specified through -J are in agreement with the JS options enabled by default in the provider when -C dynamic=true is specified.

That makes sense for now.

@saulecabrera saulecabrera changed the title Restructure the CLI interface Restructure the CLI Jul 17, 2024
saulecabrera added a commit to saulecabrera/javy that referenced this issue Aug 13, 2024
This commit introduces a `build` command in the CLI, which behaves
exactly as the compile command and introduces
a deprecation warning to the existing `compile` command.

Additionally, as a preparation for the CLI redesign discussed in
bytecodealliance#702, this commit
introduces a small refactoring of the code generation process, by:

* Introducing a code generator builder, to abstract and validate all the
  code generation option combinations.
* Introducing proper static and dynamic code generator modules, making
  it easier to divide the responsilibities of each.

  The main motivation for the refactoring is to:

  * Make it easier to finalize the CLI redesign.
  * Share code between the `compile` and `build` command while they must
    be equivalent.
  * Make it easier to keep the `compile` command frozen while it becomes
    deprecated and at the same time evolve the `build` command
    independently.

NB: Given that the `compile` and `build` command are exactly the same,
and that this change is purely mechanical; this change doesn't introduce
integration tests for the `build` command. The plan is to introduce
tests once other options are added to the `build` command as part of the
CLI redesign. Alternatively, tests can be added, however, it would
require either duplicating the entire test suite or adding testing
infrastructure to minimize duplication, which will increase the size of
the change.
saulecabrera added a commit that referenced this issue Aug 14, 2024
* Introduce the `build` command in the CLI

This commit introduces a `build` command in the CLI, which behaves
exactly as the compile command and introduces
a deprecation warning to the existing `compile` command.

Additionally, as a preparation for the CLI redesign discussed in
#702, this commit
introduces a small refactoring of the code generation process, by:

* Introducing a code generator builder, to abstract and validate all the
  code generation option combinations.
* Introducing proper static and dynamic code generator modules, making
  it easier to divide the responsilibities of each.

  The main motivation for the refactoring is to:

  * Make it easier to finalize the CLI redesign.
  * Share code between the `compile` and `build` command while they must
    be equivalent.
  * Make it easier to keep the `compile` command frozen while it becomes
    deprecated and at the same time evolve the `build` command
    independently.

NB: Given that the `compile` and `build` command are exactly the same,
and that this change is purely mechanical; this change doesn't introduce
integration tests for the `build` command. The plan is to introduce
tests once other options are added to the `build` command as part of the
CLI redesign. Alternatively, tests can be added, however, it would
require either duplicating the entire test suite or adding testing
infrastructure to minimize duplication, which will increase the size of
the change.

* Collapse `provider_version` call in `builder`

* Improve some documentation

* Clarify `source_compression` docs

* Stylistic changes

* Add NOTICE to help text of  `compile`

* Use `eprintln!` instead of `println!`

* Use `build` in the README.md
saulecabrera added a commit to saulecabrera/javy that referenced this issue Aug 14, 2024
This commit refactors the integration tests to use the `javy-runner`
crate.

The main motivation for this change is to reduce duplication and make it
easier to test more complex scenarios in the future e.g., the upcoming
changes in bytecodealliance#702
saulecabrera added a commit that referenced this issue Aug 14, 2024
* Refactor integration tests

This commit refactors the integration tests to use the `javy-runner`
crate.

The main motivation for this change is to reduce duplication and make it
easier to test more complex scenarios in the future e.g., the upcoming
changes in #702

* Style changes

* More style changes
@konradkoschel
Copy link

Hi Saúl,

I have one question (or feature request) regarding the CLI refactoring. As the Javy-CLI npm package was recently deprecated, would it be possible to release the javy-cli crate on crates.io similar to the javy crate itself?

Ideally, the crate would also include a lib.rs to be consumable as a library. I think this would be comparably simple to implement.

Technically, as a fully-fledged replacement for the npm package, a new wrapper using napi.rs could be created, where the workaround (downloading the binary from Github and executing it in a child-process) is no longer needed. I see the point that this does introduce a lot more implementation efforts for sure though.

Would love to hear your opinion on these suggestions 😀

I'd be open to support this also.

@jeffcharles
Copy link
Collaborator

Would you be open to describing how you were using the javy-cli NPM package and how you would use a Rust crate around the CLI layer? I just want to understand the use case better.

@konradkoschel
Copy link

Would you be open to describing how you were using the javy-cli NPM package and how you would use a Rust crate around the CLI layer? I just want to understand the use case better.

Hi Jeff,

Sure!

I am creating tooling around the Javy CLI. This means that I create a wrapper wherein as part of the overarching build also the Javy CLI is used.
As this wrapper is an npm package, we used the now deprecated javy-cli npm package to only download the appropriate platform-specific binary instead of vendoring all of them and routing function calls at runtime.

As the Javy-CLI is used as one part in a bigger build process, we effectively use it as a library which the existing npm package kind of simulated (as it effectively delegated the function calls to child process invocations of the binary builds).

Such use cases would be much simplified if the APIs the Javy CLI provides as an executable were also available as a library. And of course, publishing the CLI crate on crates.io would be very helpful as it could then be used as a dependency.

In my particular (and surely special) case where ultimately an npm package is created, I could set the javy-cli-lib crate as a dependency in a project where napi.rs or a different technology is used to transform it into a native Node Addon.
If javy-cli is published as a crate, this can be done independently and without "polluting" the main Javy repository. Without, only forking/vendoring or techniques as used in the deprecated package are possible.

From technological point of view, it should also be quite easy as Rust allows one library crate and one-to-many binary crates per project. If lib.rs and main.rs are implemented well, then they could be small layers on top of a shared core ensuring consistent APIs and very little extra maintenance. Arguably this kind of separation is already mostly achieved in the code.

Also for using the Javy CLI standalone – without a wrapper use case – publishing it is beneficial as consumers could simply install it via cargo cargo install javy-cli, upgrade it and use it.
Downloading the binaries manually works fine but certainly is a little bit less convenient due to manual upgrades, PATH handling, etc.

I hope this describes the use case and motivation a bit better, otherwise feel free to come back to me. I am happy to discuss :)

@jeffcharles
Copy link
Collaborator

jeffcharles commented Aug 20, 2024

Okay, it sounds like you use a Javy binary inside a cross-platform Node (or Node-API compatible) application and would like a way to get either a single binary that's compatible for whatever the current platform is or get an embeddable library you can call into. Is that correct?

it should also be quite easy

It's actually a bit more complicated 😅. Cargo/Rust don't really have a great way of expressing that a Rust crate targeting a native architecture has a dependency on a Rust crate that should be compiled to Wasm as is the case with a native javy-cli crate depending on javy-core compiled to Wasm. As such, the javy-cli Rust crate build process currently requires the library version and the command version of javy-core Rust crate compiled to WASI preview 1 to exist on the filesystem in particular locations. For the time being, we use a Makefile to model this. A build script in javy-cli isn't recommended to model this because it would involve Cargo recursively calling itself to build the core crate into the two artifacts first.

@saulecabrera
Copy link
Member Author

Hi @konradkoschel -- just wanted to add a bit to what Jeff mentioned above. In general, I agree that it would be useful to have the javy-cli published in crates.io, given that:

  1. We've deprecated the javy-cli
  2. It'd be ideal to be able to cargo install javy-cli
  3. Publishing a Rust version the CLI, rather than an npm wrapper requires less maintenance effort to keep features in sync.

As mentioned in the previous comment, the biggest wrinkle here is probably figuring out a way to model the dependency between the javy-cli crate and the Wasm artifact produced by the javy crate. Something like the artifact dependency RFC would come handy in this situation, unfortunately, it's still a work-in-progress. There are work arounds that are maybe worth trying, like having a custom pre-publish script to ensure that all the artifact dependencies are met.

In general I wanted to say that I'm personally in favor of publishing the CLI to crates.io. However, I don't have the bandwidth to take this feature on at the moment, but we welcome contributions. If you'd like I can help guide the development and review PRs. If that's the case, a good first step could to open an issue so that we can discuss and agree on a path forward.

@konradkoschel
Copy link

Thank you two for the answers! I fully understand the issue with the WASM dependency – this makes it a lot more complicated than thought for sure.

I'll make up my mind on how to best address this and if also my bandwidth allows it, I'll try to prepare a PR 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants