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.

Sign up for GitHub

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

Jump to bottom

(v7.x backport) src: allow CLI args in env with NODE_OPTIONS #12648

Merged
merged 1 commit into from
May 16, 2017
Merged

(v7.x backport) src: allow CLI args in env with NODE_OPTIONS #12648

merged 1 commit into from
May 16, 2017

Conversation

sam-github
Copy link
Contributor

See #12647, needed backport because of a trivial conflict against #10156 which touched the same long, long line in vcbuild.bat.

Not all CLI options are supported, those that are problematic from a
security or implementation point of view are disallowed, as are ones
that are inappropriate (for example, -e, -p, --i), or that only make
sense when changed with code changes (such as options that change the
javascript syntax or add new APIs).

PR-URL: #12028
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Michael Dawson michael_dawson@ca.ibm.com
Reviewed-By: Refael Ackermann refack@gmail.com
Reviewed-By: Gibson Fahnestock gibfahn@gmail.com
Reviewed-By: Bradley Farias bradley.meck@gmail.com

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
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. v7.x windows Issues and PRs related to the Windows platform. labels Apr 25, 2017
@sam-github sam-github added lts-agenda semver-minor PRs that contain new features and should be released in the next minor version. labels Apr 25, 2017
@sam-github
Copy link
Contributor Author

sam-github commented Apr 25, 2017 •
edited
Loading

I request that this be landed on the next 7.x that is accepting minors. @nodejs/lts

I'm also going to request backporting to 6.x once its been in 7.x long enough to prove itself, but 6.x has non-trivial conflicts, I have to work through the actual C++ code and re-apply the changes. I'm doing that now.

@evanlucas evanlucas force-pushed the v7.x-staging branch 2 times, most recently from d4ceb59 to 69a8053 Compare May 3, 2017 12:56
@sam-github sam-github mentioned this pull request May 9, 2017
Closed
@gibfahn
Copy link
Member

gibfahn commented May 10, 2017 •
edited
Loading

Needs a rebase

@sam-github
Copy link
Contributor Author

rebased without conflicts

jasnell
jasnell approved these changes May 10, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green.

@jasnell
Copy link
Member

jasnell commented May 10, 2017

The v7.x branch is not an LTS branch so this does not need to be on the LTS WG agenda.

@sam-github
src: allow CLI args in env with NODE_OPTIONS
34e8eb6 
Not all CLI options are supported, those that are problematic from a
security or implementation point of view are disallowed, as are ones
that are inappropriate (for example, -e, -p, --i), or that only make
sense when changed with code changes (such as options that change the
javascript syntax or add new APIs).

PR-URL: nodejs#12028
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
@sam-github sam-github merged commit 34e8eb6 into nodejs:v7.x-staging May 16, 2017
@sam-github sam-github deleted the backport-12028-to-v7.x branch May 16, 2017 19:45
@sam-github
Copy link
Contributor Author

sam-github commented May 18, 2017 •
edited
Loading

@jasnell I don't know what I was thinking (I think I got confused between the 3 or 4 PRs I was doing CI runs on), but I landed this without CI, and its broken. Can I force-push it off of v7.x-staging? Its currently the head commit.

@sam-github sam-github mentioned this pull request May 18, 2017
Merged
4 tasks
@sam-github
Copy link
Contributor Author

Got an OK from @evanlucas on IRC, and force-pushed this of of v7.x-staging. Trying again in #13063

@aliscco aliscco mentioned this pull request Apr 27, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sam-github @gibfahn @jasnell @nodejs-github-bot