cli: allow --jitless V8 flag in NODE_OPTIONS by andrewdotn · Pull Request #32100 · nodejs/node · GitHub

This work is modeled on #30094 which allowed
--disallow-code-generation-from-strings in NODE_OPTIONS.

The --jitless v8 option has been supported since 12.0.0. As a v8 option,
node automatically picks it up, but there have been a few issues that were
resolved by simply telling users about the option: #26758, #28800.

This PR:

allows --jitless in NODE_OPTIONS
documents --jitless
moves --experimental-loader=module to locally restore alphabetical
order in option documentation

Refs: #30094
Refs: #26758
Refs: #28800

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

This work is modeled on nodejs#30094 which allowed `--disallow-code-generation-from-strings` in `NODE_OPTIONS`. The `--jitless` v8 option has been supported since 12.0.0. As a v8 option, node automatically picks it up, but there have been a few issues that were resolved by simply telling users about the option: nodejs#26758, nodejs#28800. This PR: - allows `--jitless` in `NODE_OPTIONS` - documents `--jitless` - moves `--experimental-loader=module` to locally restore alphabetical order in option documentation Refs: nodejs#30094 Refs: nodejs#26758 Refs: nodejs#28800

LGTM. @joyeecheung do you have any concerns about documenting this flag?

I'm OK with docing things that genuinely solve problems, otherwise they are undiscoverable by users. A reorg of the docs so v8 flags are in their own section with a specific (in)stability guarantee would be great (any takers?), but I don't think should block this.

@sam-github i'd be happy to try to reorg the docs if i can find some time upcoming

v8 releases a new stable version roughly [every six weeks][], with the schedule corresponding to releases of new chrome versions. v8 maintenance of the previous stable version stops immediately, so Node.js minor versions that pull in the latest stable v8 may have arbitrary changes. [every six weeks]: https://v8.dev/docs/release-process

LGTM

It’s great to see so many positive reviews for this PR; thanks everyone.

I’d like to see it merged; what do I need to do for that to happen?

CI: https://ci.nodejs.org/job/node-test-pull-request/29664/

This work is modeled on #30094 which allowed `--disallow-code-generation-from-strings` in `NODE_OPTIONS`. The `--jitless` v8 option has been supported since 12.0.0. As a v8 option, node automatically picks it up, but there have been a few issues that were resolved by simply telling users about the option: #26758, This PR: - allows `--jitless` in `NODE_OPTIONS` - documents `--jitless` - moves `--experimental-loader=module` to locally restore alphabetical order in option documentation Refs: #30094 Refs: #26758 Refs: #28800 PR-URL: #32100 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Shelley Vohr <codebytere@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Landed in 9e69d97

Thank you for merging this.

This work is modeled on #30094 which allowed `--disallow-code-generation-from-strings` in `NODE_OPTIONS`. The `--jitless` v8 option has been supported since 12.0.0. As a v8 option, node automatically picks it up, but there have been a few issues that were resolved by simply telling users about the option: #26758, This PR: - allows `--jitless` in `NODE_OPTIONS` - documents `--jitless` - moves `--experimental-loader=module` to locally restore alphabetical order in option documentation Refs: #30094 Refs: #26758 Refs: #28800 PR-URL: #32100 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Shelley Vohr <codebytere@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable changes: * async_hooks: - add sync enterWith to ALS (Stephen Belanger) #31945 * cli: - allow --jitless V8 flag in NODE\_OPTIONS (Andrew Neitsch) #32100 * fs: - return first folder made by mkdir recursive (Benjamin Coe) #31530 * n-api: - define release 6 (Gabriel Schulhof) #32058 * src: - create a getter for kernel version (Juan José Arboleda) #31732 * wasi: - add returnOnExit option (Colin Ihrig) #32101 PR-URL: #32185

This work is modeled on #30094 which allowed `--disallow-code-generation-from-strings` in `NODE_OPTIONS`. The `--jitless` v8 option has been supported since 12.0.0. As a v8 option, node automatically picks it up, but there have been a few issues that were resolved by simply telling users about the option: #26758, This PR: - allows `--jitless` in `NODE_OPTIONS` - documents `--jitless` - moves `--experimental-loader=module` to locally restore alphabetical order in option documentation Refs: #30094 Refs: #26758 Refs: #28800 PR-URL: #32100 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Shelley Vohr <codebytere@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Should this be Semver-Minor?

@MylesBorins I don’t think we need to count this as adding a new API, so I’m good with treating this as semver-patch (especially if you’re concerned about including this in 12.16.2 as a patch release).

@addaleax ok that's good to know. We have a failure on AIX we are tracking down and suspect this might be the cause. @codebytere is pulling this out of 12.x-staging for right now to see if we can get green CI. if This is indeed the cause of the failures then we may need to open a backport to ensure things can pass. Will keep this PR updated

@MylesBorins Can you point me towards that failure? It seems highly unlikely that this would cause issues, but if it does, this should be pretty straightforward to identify (by running node --jitless on an AIX machine).

Hm yeah, this is in fact what’s causing the failures on AIX.

I’d personally recommend skipping the added line from the test here on v12.x only, rather than excluding it from the release altogether.

This work is modeled on nodejs#30094 which allowed `--disallow-code-generation-from-strings` in `NODE_OPTIONS`. The `--jitless` v8 option has been supported since 12.0.0. As a v8 option, node automatically picks it up, but there have been a few issues that were resolved by simply telling users about the option: nodejs#26758, This PR: - allows `--jitless` in `NODE_OPTIONS` - documents `--jitless` - moves `--experimental-loader=module` to locally restore alphabetical order in option documentation Refs: nodejs#30094 Refs: nodejs#26758 Refs: nodejs#28800 PR-URL: nodejs#32100 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Shelley Vohr <codebytere@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

It seems like removing this PR indeed fixed AIX CI.

https://ci.nodejs.org/job/node-test-commit-aix/29400/nodes=aix71-ppc64/testReport/(root)/test/parallel_test_cli_node_options/ is an example of the failure

I've opened up a backport PR and kicked off CI, maybe that will help

#32594

There’s already one instance of a conditional in that test file, skipping testing on ARM of an option that’s not supported on ARM:

https://github.com/nodejs/node/blob/master/test/parallel/test-cli-node-options.js#L77-L79

// Unsupported on arm. See https://crbug.com/v8/8713.
if (!['arm', 'arm64'].includes(process.arch))
  expect('--interpreted-frames-native-stack', 'B\n');

Does the --jitless line need a not-AIX check?

(I’m assuming it’s not feasible for a random developer to easily get shell access to an AIX box for testing, but please correct me if I’m wrong about that.)

@andrewdotn See #32594 (comment) :)

@addaleax Ah, thanks!

This work is modeled on #30094 which allowed `--disallow-code-generation-from-strings` in `NODE_OPTIONS`. The `--jitless` v8 option has been supported since 12.0.0. As a v8 option, node automatically picks it up, but there have been a few issues that were resolved by simply telling users about the option: #26758, This PR: - allows `--jitless` in `NODE_OPTIONS` - documents `--jitless` - moves `--experimental-loader=module` to locally restore alphabetical order in option documentation Refs: #30094 Refs: #26758 Refs: #28800 Backport-PR-URL: #32594 PR-URL: #32100 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Shelley Vohr <codebytere@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 5, 2020

richardlau approved these changes Mar 5, 2020

View reviewed changes

jasnell reviewed Mar 5, 2020

View reviewed changes

Comment thread doc/api/cli.md

cjihrig approved these changes Mar 5, 2020

View reviewed changes

sam-github approved these changes Mar 5, 2020

View reviewed changes

devsnek approved these changes Mar 5, 2020

View reviewed changes

codebytere approved these changes Mar 5, 2020

View reviewed changes

devnexen approved these changes Mar 5, 2020

View reviewed changes

addaleax approved these changes Mar 5, 2020

View reviewed changes

jasnell approved these changes Mar 6, 2020

View reviewed changes

Fix issue from doc linting

fbfa95d

mhdawson approved these changes Mar 6, 2020

View reviewed changes

BridgeAR approved these changes Mar 9, 2020

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 9, 2020

mmarchini closed this Mar 9, 2020

MylesBorins mentioned this pull request Mar 10, 2020

v13.11.0 proposal #32185

Merged

codebytere mentioned this pull request Mar 24, 2020

v12.16.2 proposal #32313

Merged

MylesBorins mentioned this pull request Apr 1, 2020

[v12.x] cli: allow --jitless V8 flag in NODE_OPTIONS #32594

Closed

MylesBorins added the backport-open-v12.x label Apr 1, 2020

MylesBorins mentioned this pull request Apr 1, 2020

[v12.x] --jitless not working on aix on v12.x #32600

Closed

addaleax added backported-to-v12.x and removed backport-open-v12.x labels Sep 23, 2020

cli: allow --jitless V8 flag in NODE_OPTIONS by andrewdotn · Pull Request #32100 · nodejs/node · GitHub

Uh oh!

Uh oh!

cli: allow --jitless V8 flag in NODE_OPTIONS - #32100

cli: allow --jitless V8 flag in NODE_OPTIONS#32100andrewdotn wants to merge 3 commits intonodejs:masternodejs/node:masterfrom andrewdotn:masterandrewdotn/node:masterCopy head branch name to clipboard

Conversation

andrewdotn commented Mar 5, 2020

Checklist

Uh oh!

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

codebytere left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

andrewdotn commented Mar 9, 2020

Uh oh!

nodejs-github-bot commented Mar 9, 2020

Uh oh!

mmarchini commented Mar 9, 2020

Uh oh!

andrewdotn commented Mar 9, 2020

Uh oh!

MylesBorins commented Mar 31, 2020

Uh oh!

addaleax commented Mar 31, 2020

Uh oh!

MylesBorins commented Mar 31, 2020

Uh oh!

addaleax commented Apr 1, 2020

Uh oh!

addaleax commented Apr 1, 2020

Uh oh!

MylesBorins commented Apr 1, 2020

Uh oh!

andrewdotn commented Apr 1, 2020

Uh oh!

addaleax commented Apr 1, 2020

Uh oh!

andrewdotn commented Apr 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

cli: allow --jitless V8 flag in NODE_OPTIONS#32100
andrewdotn wants to merge 3 commits into
nodejs:masterfrom
andrewdotn:master