-
Notifications
You must be signed in to change notification settings - Fork 109
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
Add JS runtime option infra and redirect-stdout-to-stderr option #739
Add JS runtime option infra and redirect-stdout-to-stderr option #739
Conversation
The existing fuel consumption number on main is 37,309 for the log test is when compiled run on my Macbook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me. Left one comment that I think makes sense to address before landing this one!
crates/cli/src/main.rs
Outdated
let mut gen = if codegen.dynamic { | ||
builder.build::<DynamicGenerator>()? | ||
if js_runtime_options != JsRuntimeOptionGroup::default() { | ||
bail!("Cannot set JS runtime options when building a dynamic module"); | ||
} | ||
builder.build_dynamic()? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a second thought, I wonder if the builder is better suited to perform this validation?
That way:
- We can keep the current
build
method, shared across dynamic/static codegen. - Perform the validation of the option groups through
javy::Config
- Keep main as slim as possible i.e., avoid baking in builder specific logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words, we could modify the existing build method signature to be:
fn build<T>(config: javy_config::Config) -> Result<..>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point in the future, if the defaults for build
and compile
diverge, the logic for validating the config for dynamic is going to depend on whether the build or compile command was used. But we can deal with that case when it actually comes up I guess.
Description of the change
The only functional change in this PR is adding support for
-J redirect-stdout-to-stderr[=y|n]
with a default ofy
tojavy build
. There are no other functional changes and behaviour without specifying that flag doesn't change since the existing code usesy
for that setting implicitly.At the code level, there's a few related changes in this PR:
build
functions instead of a single shared function so I can pass an argument to the static codegen build function and not to the dynamic build function. This involved deleting some code that is no longer referenced.JsRuntimeOptionGroup
struct, aJsRuntimeOption
enum, and trait implementations to convert aVec<JsRuntimeOption>
to aJsRuntimeOptionGroup
, and from aJsRuntimeOptionGroup
to ajavy_config::Config
so we have a way of representing command line options for JS runtime settings.JsRuntimeOptionGroup
struct to generate thejavy_config::Config
instead of usingConfig::all()
and pass it into the codegen struct.Default
for theCodegenOptionGroup
and use the defaults for converting fromVec<CodegenOption>
toCodegenOptionGroup
to keep it consistent with howJsRuntimeOptionGroup
works.Why am I making this change?
See #702. I'm implementing this similarly to how I did for the codegen options. I'm also opting to return an error when generating a dynamic module if any of the JS runtime options are set to non-default values since those runtime options won't be applied to the generated module (the provider defines the runtime options). The refactoring of the codegen builder was because I wanted to try to make invalid state non-representable in the non-test code.
I can probably break this PR up if it's too large. I also tried avoiding any functional changes that I didn't have to so the PR could be kept smaller. Hence not bumping the provider version yet.
Checklist
javy-cli
andjavy-core
do not require updating CHANGELOG files.