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

Allow specifying which file descriptor is used in console.{log,err} from the CLI #628

Closed
Niikelion opened this issue Apr 13, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@Niikelion
Copy link

Operating system: windows + wsl2-ubuntu, windows
Processor architecture: x86_64
Rust version: rustup 1.27.0
Javy version: javy 1.4.0

Problem

After compiling console.log("test") into .wasm file and executing it with wasmtime I get output on stderr instead of stdout.
I verified that its not an wasmtime problem by compiling and running simple c program with printf("test\n"). Testing was done in windows and wsl2 with ubuntu on windows and in happens in both cases.

@Niikelion Niikelion added the bug Something isn't working label Apr 13, 2024
@saulecabrera
Copy link
Member

It indeed does. I believe we should make this behavior configurable from the CLI level too, because currently it's not standard. For some context, some use-cases require tight control over which stream is used for console.{log,err}, however, in this case the non-standard behavior has "leaked" as the standard one.

@saulecabrera
Copy link
Member

I'll switch the label and title of the issue to reflect that we want to make this behavior configurable.

@saulecabrera saulecabrera added enhancement New feature or request and removed bug Something isn't working labels Apr 15, 2024
@saulecabrera saulecabrera changed the title console.log prints to stderr Allow specifying which file descriptor is used in console.{log,err} from the CLI Apr 15, 2024
@saulecabrera saulecabrera changed the title Allow specifying which file descriptor is used in console.{log,err} from the CLI Allow specifying which file descriptor is used in console.{log,err} from the CLI Apr 15, 2024
saulecabrera added a commit to saulecabrera/javy that referenced this issue May 9, 2024
This change primarily rewrites the current APIs using "Intrinsics" as defined by rquickjs.

This change has several objectives:

Refactor the structure of the Config and its relationship with the Runtime to pave the way for a truly extensible and configurable engine. The way Intrinsics are exposed in rquickjs simplifies (i) enabling a granular configuration experience of the native intrinsics, which was not entirely clear how to achieve before, and (ii) adding custom APIs as intrinsics.
Enhance how our configuration operates: the previous model assumed an almost static configuration setup, allowing only limited, non-straightforward customizability from the CLI in the future. This was mostly modeled per API and not as a global aspect of the runtime. Our recommended approach for users needing functionality beyond what was provided was to fork or recreate their own version of the CLI/APIs. A telling sign of this issue is that the configuration parameters were almost never used in any of the APIs. This PR paves the way for greater extensibility, which will be facilitated by threading configuration options from the CLI in a follow-up PR; this method will also allow for opting-in to non-standard functionality via a CLI flag, helping us avoid issues like the one noted here: bytecodealliance#628.

Along the way,  I had to make several changes to make all this work; the most relevant ones  are: (i)  deprecating the `javy-apis` crate (ii) enabling all the APIs by default and (iii) reworking a bit the `Console` implementation. 

The reasoning to deprecate the `javy-apis` crate is mostly around: (i) the fact that we're revamping our extensilbity model in the near fuiture and (ii)  the complexity around cyclical dependencies:  `javy` exposes rquickjs and given that the cofiguration is tied to the runtime, `javy` needed to depend on `javy-apis` but the other way around was also true. I decided not to spend too much cycles on this mostly given point (i) above. 

There still things to follow-up on after this PR, namely 

- [ ] Update all the documentation under  `docs` 
- [ ] A PR to thread the engine configuration from the CLI. NB that the engine behaviour is not  altered in this PR.
saulecabrera added a commit that referenced this issue May 14, 2024
* Refactor the structure of the `Config`/APIs

This change primarily rewrites the current APIs using "Intrinsics" as defined by rquickjs.

This change has several objectives:

Refactor the structure of the Config and its relationship with the Runtime to pave the way for a truly extensible and configurable engine. The way Intrinsics are exposed in rquickjs simplifies (i) enabling a granular configuration experience of the native intrinsics, which was not entirely clear how to achieve before, and (ii) adding custom APIs as intrinsics.
Enhance how our configuration operates: the previous model assumed an almost static configuration setup, allowing only limited, non-straightforward customizability from the CLI in the future. This was mostly modeled per API and not as a global aspect of the runtime. Our recommended approach for users needing functionality beyond what was provided was to fork or recreate their own version of the CLI/APIs. A telling sign of this issue is that the configuration parameters were almost never used in any of the APIs. This PR paves the way for greater extensibility, which will be facilitated by threading configuration options from the CLI in a follow-up PR; this method will also allow for opting-in to non-standard functionality via a CLI flag, helping us avoid issues like the one noted here: #628.

Along the way,  I had to make several changes to make all this work; the most relevant ones  are: (i)  deprecating the `javy-apis` crate (ii) enabling all the APIs by default and (iii) reworking a bit the `Console` implementation. 

The reasoning to deprecate the `javy-apis` crate is mostly around: (i) the fact that we're revamping our extensilbity model in the near fuiture and (ii)  the complexity around cyclical dependencies:  `javy` exposes rquickjs and given that the cofiguration is tied to the runtime, `javy` needed to depend on `javy-apis` but the other way around was also true. I decided not to spend too much cycles on this mostly given point (i) above. 

There still things to follow-up on after this PR, namely 

- [ ] Update all the documentation under  `docs` 
- [ ] A PR to thread the engine configuration from the CLI. NB that the engine behaviour is not  altered in this PR.

* Fix typos

* Add a comment in the apis/lib.rs 

This makes `cargo fmt -- --check` happy

* Review comments
@saulecabrera
Copy link
Member

In the latest main, it's now possible to override this behavior, by doing javy build index.js -J redirect-stdout-to-stderr=n -o index.wasm

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

2 participants