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

CLI: Allow Input from stdin and Output to stdout #701

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

konradkoschel
Copy link

@konradkoschel konradkoschel commented Jul 11, 2024

Description of the change

In this PR, the CLI crate is enhanced so that a hyphen (-) can be passed for the input argument or the output option instead of a file name indicating that stdio streams should be used.

Possible usage

# Use stdin for input and file for output
echo "console.log('Hello World!');" | javy compile - -o my.wasm

# Pipe wasm output into another command
javy compile my.js -o - | wasm-opt ...

# Use no files at all
echo "console.log('Hello World!');" | javy compile - -o - | wasm-opt ...

Why am I making this change?

I built a wrapper around Javy. The wrapper is not inherently working with files, but to make it work with Javy, I need to write the JavaScript to the filesystem and read the WASM from the filesystem.

Generally, the changes make the CLI of Javy more flexible.

Checklist

  • I've updated the relevant CHANGELOG files if necessary. Changes to javy-cli and javy-core do not require updating CHANGELOG files.
  • I've updated the relevant crate versions if necessary. Versioning policy for library crates
  • I've updated documentation including crate documentation if necessary.

@konradkoschel konradkoschel marked this pull request as ready for review July 11, 2024 15:52
Copy link
Member

@saulecabrera saulecabrera 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 the PR -- generally these changes seem reasonable to me. One thing to note is that we're currently thinking of revamping Javy's CLI interface (see #702).

Given that our intention is to move in the direction described in the issue linked above, it probably makes sense to include this change as part of the new build command. That said, you're not expected to take on the CLI changes, for this change to land. However, I'd like to ask if you're ok waiting until we at least introduce the command that will replace the current compile command in order to merge this?

@@ -21,7 +21,14 @@ fn main() -> Result<()> {
match args.command {
Command::EmitProvider(opts) => emit_provider(&opts),
Command::Compile(opts) => {
let js = JS::from_file(&opts.input)?;
let js = match opts.input.to_str() {
Some("-") => {
Copy link
Member

@saulecabrera saulecabrera Jul 11, 2024

Choose a reason for hiding this comment

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

On the technical side of this change, any particular reason why you chose hyphens over:

  • Declaring the input and output options as optional via Option<T>
  • Default to stdin / stdout accordingly if either or both are not specified

Copy link
Author

Choose a reason for hiding this comment

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

For stdin input, I would argue that it is common practice to use a hyphen as the argument to indicate this explicitely. For the output, I didn't want to break the current default behavior (i.e. using index.wasm).

I see the point though, that especially the combination of both (-o - - / - -o -) may look a bit weird. In the end, I wanted to primarily make a proposal for the functionality – the implementation details can be discussed.

As this will anyway be implemented as part of the new build command, I would default the output to stdout as suggested by you.

@konradkoschel
Copy link
Author

Thanks for the PR -- generally these changes seem reasonable to me. One thing to note is that we're currently thinking of revamping Javy's CLI interface (see #702).

Given that our intention is to move in the direction described in the issue linked above, it probably makes sense to include this change as part of the new build command. That said, you're not expected to take on the CLI changes, for this change to land. However, I'd like to ask if you're ok waiting until we at least introduce the command that will replace the current compile command in order to merge this?

Hi, thanks for the fast response! I agree on the point that it makes sense to incorporate the functionality into the upcoming build command.
From my point of view, the option to use the standard streams as input and output for Javy is a nice addition, but it is not too urgent.
So I can surely wait :)

@saulecabrera saulecabrera mentioned this pull request Jul 15, 2024
8 tasks
@saulecabrera
Copy link
Member

Hi @konradkoschel, wanted to let you know that most of the backing infrastructure for the CLI redesign has landed on main, and I think it's now a good time to rebase, update and move forward with this change. Let me know if you have any questions.

@konradkoschel
Copy link
Author

Hi @konradkoschel, wanted to let you know that most of the backing infrastructure for the CLI redesign has landed on main, and I think it's now a good time to rebase, update and move forward with this change. Let me know if you have any questions.

Hi Saul,

I updated the branch. Could you check if the code changes are fine?

As discussed previously, I made the binary output the default instead of -o - as in the original implementation. Tests run successful on my side

Copy link
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

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

Code changes look good to me. Would you mind adding a test or two here https://github.com/bytecodealliance/javy/blob/main/crates/cli/tests/integration_test.rs to test the new behavior?

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.

2 participants