OCamlPro / owi

WebAssembly Swissknife & cross-language bugfinder
https://ocamlpro.github.io/owi/
GNU Affero General Public License v3.0
137 stars 18 forks source link

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

Closed vasucp1207 closed 3 months ago

vasucp1207 commented 3 months ago

Emit wasm files in same dir as wat files

zapashcanon commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

Yes sure, so I propose the following changes:

vasucp1207 commented 3 months ago
  • 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.

zapashcanon commented 3 months ago

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

vasucp1207 commented 3 months ago

@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

zapashcanon commented 3 months ago

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 commented 3 months ago

Thanks for your patience! :)