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

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

Closed
wants to merge 3 commits into from

Conversation

andrewdotn
Copy link
Contributor

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
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 5, 2020
doc/api/cli.md Show resolved Hide resolved
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

@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
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewdotn
Copy link
Contributor Author

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?

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 9, 2020
@nodejs-github-bot
Copy link
Collaborator

mmarchini pushed a commit that referenced this pull request Mar 9, 2020
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@mmarchini
Copy link
Contributor

Landed in 9e69d97

@mmarchini mmarchini closed this Mar 9, 2020
@andrewdotn
Copy link
Contributor Author

Thank you for merging this.

MylesBorins pushed a commit that referenced this pull request Mar 9, 2020
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 10, 2020
MylesBorins added a commit that referenced this pull request Mar 11, 2020
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
MylesBorins added a commit that referenced this pull request Mar 11, 2020
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
MylesBorins added a commit that referenced this pull request Mar 12, 2020
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
MylesBorins added a commit that referenced this pull request Mar 12, 2020
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
codebytere pushed a commit that referenced this pull request Mar 23, 2020
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@codebytere codebytere mentioned this pull request Mar 24, 2020
codebytere pushed a commit that referenced this pull request Mar 30, 2020
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins
Copy link
Contributor

Should this be Semver-Minor?

@addaleax
Copy link
Member

@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).

@MylesBorins
Copy link
Contributor

@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

@addaleax
Copy link
Member

addaleax commented Apr 1, 2020

@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).

@addaleax
Copy link
Member

addaleax commented Apr 1, 2020

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.

MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Apr 1, 2020
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins
Copy link
Contributor

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

@andrewdotn
Copy link
Contributor Author

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.)

@addaleax
Copy link
Member

addaleax commented Apr 1, 2020

@andrewdotn See #32594 (comment) :)

@andrewdotn
Copy link
Contributor Author

@addaleax Ah, thanks!

MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.