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

fix: add --output to wat2wasm, wasm2wat and opt cmd #416

Merged
merged 6 commits into from
Aug 27, 2024

Conversation

vasucp1207
Copy link
Contributor

Emit wasm files in same dir as wat files

@zapashcanon
Copy link
Member

Also, I'm not sure we really want this behaviour, I checked and the wat2wasm command from wabt actually outputs the file in the current folder. @filipeom, @krtab WDYT?

@vasucp1207
Copy link
Contributor Author

Also, I'm not sure we really want this behaviour, I checked and the wat2wasm command from wabt actually outputs the file in the current folder. @filipeom, @krtab WDYT?

Yes, you are right, because the wasm2wat emit the files in the same folder that's why I did this.

@zapashcanon
Copy link
Member

Also, I'm not sure we really want this behaviour, I checked and the wat2wasm command from wabt actually outputs the file in the current folder. @filipeom, @krtab WDYT?

Yes, you are right, because the wasm2wat emit the files in the same folder that's why I did this.

Aw OK. Maybe we could have a flag then, something like --emit-file-in-current-directory for the same behavior as wat2wasm and keep the new one as default; or --emit-file-in-same-directory to get the new one and keep the wat2wasm one as default ?

@filipeom
Copy link
Collaborator

Also, I'm not sure we really want this behaviour, I checked and the wat2wasm command from wabt actually outputs the file in the current folder. @filipeom, @krtab WDYT?

I understand why this PR is needed, but I'm not entirely in favor of it. I believe both --emit-file-in-current-directory and --emit-file-in-same-directory are adequate solutions.

However, I must confess that I prefer the wat2wasm approach: passing in one file and exporting it to another using -o. When I need to process multiple files, I can simply iterate through them in a shell loop. I think accepting a list of files as input is less flexible, as there might be use cases where someone wants each file to be exported to a different destination.

I'll leave the final decision to someone with a stronger opinion.

@vasucp1207
Copy link
Contributor Author

vasucp1207 commented Aug 23, 2024

However, I must confess that I prefer the wat2wasm approach: passing in one file and exporting it to another using -o. When I need to process multiple files, I can simply iterate through them in a shell loop. I think accepting a list of files as input is less flexible, as there might be use cases where someone wants each file to be exported to a different destination.

I'll leave the final decision to someone with a stronger opinion.

I agree that accepting a single file as an argument is a good idea for specifying the target path, let wat2wasm be the default approach and I suppose there is no need for these type of flags --emit-file-in-same-directory.

@zapashcanon
Copy link
Member

@vasucp1207, do you want to make the changes to accepting a single file, to use the current directory and to accept the -o option? :)

@vasucp1207
Copy link
Contributor Author

@vasucp1207, do you want to make the changes to accepting a single file, to use the current directory and to accept the -o option? :)

Yup, wabt also accepts a single file.

@zapashcanon
Copy link
Member

zapashcanon commented Aug 23, 2024

Yes sure, so I propose the following changes:

  • make owi wat2wasm and owi wasm2wat accept only one file;
  • add a -o FILE option to both (we can keep --emit-file too for owi wat2wasm);
  • make the default output directory be the current directory (when -o FILE is not present).

@vasucp1207
Copy link
Contributor Author

  • make the default output directory be the current directory (when -o FILE or --emit-file is not present).

Not include the --emit-file option to wat2wasm because it's by default emits the file anyway.

@vasucp1207 vasucp1207 changed the title fix: wat2wasm emit files files in same dir fix: add --output to wat2wasm and wasm2wat Aug 26, 2024
@zapashcanon
Copy link
Member

@vasucp1207, I believe we also have to do the same for owi opt along the way

@vasucp1207 vasucp1207 changed the title fix: add --output to wat2wasm and wasm2wat fix: add --output to wat2wasm, wasm2wat and opt cmd Aug 26, 2024
@vasucp1207
Copy link
Contributor Author

@vasucp1207, I believe we also have to do the same for owi opt along the way

Sure, but I didn't add --emit-file to owi opt because of name conflicts if the source file is in the current dir

src/cmd/cmd_opt.ml Outdated Show resolved Hide resolved
CHANGES.md Outdated
@@ -1,5 +1,6 @@
## unreleased

- `wat2wasm` emit wasm files in same dir as wat files
Copy link
Member

Choose a reason for hiding this comment

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

This should be updated now :)

src/bin/owi.ml Outdated Show resolved Hide resolved
src/bin/owi.ml Outdated Show resolved Hide resolved
@zapashcanon
Copy link
Member

zapashcanon commented Aug 27, 2024

We are getting there! Don't forget to run and promote the tests once the last issues have been fixed so that I can merge directly. :)

Also, could you add a test for each command (they each have a directory in test/) ? It could be as simple as:

$ owi {opt,wat2wasm,wasm2wat} foo --output=bar
$ owi fmt bar

@zapashcanon
Copy link
Member

Thanks for your patience! :)

@zapashcanon zapashcanon merged commit aef0bd6 into OCamlPro:main Aug 27, 2024
1 of 4 checks passed
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.

3 participants