fix: enable use of NODE_OPTIONS env var #15158

codebytere · 2018-10-14T09:45:26Z

Description of Change

Enables use of NODE_OPTIONS environment variable in Electron.

Depends on electron/node#76.

Potential concerns: options relating to V8 that may cause collisions should they misalign with Blink's V8 usage.

/cc @deepak1556

Checklist

PR description included and stakeholders cc'd
npm test passes
PR title follows semantic commit guidelines

Release Notes

Notes: Enable use of NODE_OPTIONS env var

atom/common/node_bindings.cc

alexeykuzmin

@codebytere
We should remove NODE_WITHOUT_NODE_OPTIONS define from the e/node's BUILD.gn:
https://github.com/electron/node/blob/f14ddf9d7e07739dc1dc0cbe2f7a5ba8b44906d1/BUILD.gn#L155

alexeykuzmin · 2018-10-14T22:42:13Z

Can we add a test that NODE_OPTIONS work?

codebytere · 2018-10-15T00:17:22Z

@alexeykuzmin they do locally, will try to add a spec; also, your requested changes are a PR that was already linked in the PR body.

nornagon · 2018-10-15T19:12:47Z

What was the initial reason to remove support for NODE_OPTIONS?

codebytere · 2018-10-15T19:41:58Z

@nornagon it was never removed, it was always broken from the very beginning since we passed nullptr to node::Init which meant that when Init tried to index into argv it would segfault. A little while back we fixed the segfault by defining NODE_WITHOUT_NODE_OPTIONS, so this PR is the first attempt at actually making them work 😁

nornagon · 2018-10-15T21:06:42Z

ahh, gotcha!

deepak1556 · 2018-10-15T23:56:08Z

The changes LGTM, also needs to update for standalone node mode in atom/app/node_main.cc.

Just need some clarification, what is the benefit of enabling this for electron ? This actually removes the control of node argument parsing away from us, now some of the node options are parsed and enabled from an environment variable, which seems to bypass the blacklist we maintain when the executable is invoked through a protocol handler.

atom/common/node_bindings.cc

Because I want to

codebytere · 2018-10-17T20:59:21Z

@nornagon @alexeykuzmin @deepak1556 @nitsakh in addition to the advisory warnings above, what are your thoughts on my also explicitly disallowing all NODE_OPTIONS in packaged apps?

ckerr · 2018-10-17T20:52:36Z

atom/common/node_bindings.cc

  int exec_argc = 0;
-  const char** argv = nullptr;
+  const char* prog_name = "electron";
+  const char** argv = {&prog_name};


What are the brackets for here?

atom/common/node_bindings.cc

ckerr · 2018-10-17T20:56:34Z

atom/common/node_bindings.cc

+
+    // parse passed options for unsupported options
+    // and remove them from the options list
+    for (std::string part : parts) {


No need to create a new temporary; can use std::string& part

atom/common/node_bindings.cc

ckerr · 2018-10-17T21:00:05Z

atom/common/node_bindings.cc

+        std::size_t found = disallow.find(part);
+        if (found != std::string::npos) {
+          LOG(WARNING) << "This NODE_OPTION is not supported in Electron";
+          parts.erase(std::remove(parts.begin(), parts.end(), part),


You _really _ don't want to be modifying the parts vector while iterating through it in the outermost loop here.

I'd suggest you do something more like

std::vector<std::string> tmp; for (const auto& part : parts) { if (disallow.count(part)) { warning... } else { tmp.push_back(part); } } std::swap(parts, tmp);

ckerr · 2018-10-17T21:05:50Z

atom/common/node_bindings.cc

+        if (!is_first_round)
+          new_options << " ";
+        new_options << part;
+      }


Might try something like this:

std::copy( parts.begin(), parts.end(), std::ostream_iterator<std::string>(new_options," "));

deepak1556 · 2018-10-17T21:06:17Z

If we are disabling them completely for packaged apps and allowing otherwise, the current options parsing and warning in this PR won't be required, only a simplified warning in packaged mode would be needed to state NODE_OPTIONS don't work in this mode.

ckerr

A couple of suggestions inline, but nothing major. LGTM!

ckerr · 2018-10-18T16:30:01Z

atom/common/node_bindings.cc

+    std::transform(path.begin(), path.end(), path.begin(), ::tolower);
+
+#if defined(OS_WIN)
+    is_packaged_app = path == "electron.exe";


(minor) the advance declaration above could be removed and these two lines could be

#if defined(OS_WIN) const bool is_packaged_app = path == "electron.exe"; #else const bool is_packaged_app = path == "electron"; #endif

ckerr · 2018-10-18T16:30:38Z

atom/common/node_bindings.cc

+    if (is_packaged_app) {
+      env->SetVar("NODE_OPTIONS", "");
+    } else {
+      std::vector<std::string> disallowed = {


(minor) this can be const std::vector<std::string>

ckerr

Approving this final-draft revision on a C++ level. This code LGTM and I'd be fine with it landing.

I don't have any opinion on how to NODE_OPTIONS wrt packaging. If someone feels strongly about that, please weigh in before this merges. 😐

release-clerk · 2018-10-18T23:57:31Z

Release Notes Persisted

Enable use of NODE_OPTIONS env var

trop · 2018-10-18T23:57:45Z

We have automatically backported this PR to "4-0-x", please check out #15259

codebytere requested a review from a team October 14, 2018 09:45

codebytere mentioned this pull request Oct 14, 2018

fix: remove NODE_WITHOUT_NODE_OPTIONS electron/node#76

Merged

codebytere commented Oct 14, 2018

View reviewed changes

atom/common/node_bindings.cc Outdated Show resolved Hide resolved

codebytere force-pushed the enable-node-options branch from 0733acd to 00d87f7 Compare October 14, 2018 18:02

codebytere changed the title ~~[WIP] fix: enable use of NODE_OPTIONS~~ fix: enable use of NODE_OPTIONS Oct 14, 2018

alexeykuzmin previously requested changes Oct 14, 2018

View reviewed changes

MarshallOfSound mentioned this pull request Oct 15, 2018

chore: roll node #15177

Merged

2 tasks

codebytere closed this Oct 16, 2018

codebytere force-pushed the enable-node-options branch from ed6b88f to d3efc52 Compare October 16, 2018 17:49

codebytere reopened this Oct 16, 2018

codebytere commented Oct 16, 2018

View reviewed changes

atom/common/node_bindings.cc Outdated Show resolved Hide resolved

codebytere commented Oct 16, 2018

View reviewed changes

atom/common/node_bindings.cc Outdated Show resolved Hide resolved

nitsakh suggested changes Oct 16, 2018

View reviewed changes

atom/common/node_bindings.cc Outdated Show resolved Hide resolved

atom/common/node_bindings.cc Outdated Show resolved Hide resolved

codebytere requested a review from a team October 17, 2018 20:27

codebytere changed the title ~~fix: enable use of NODE_OPTIONS~~ fix: enable use of NODE_OPTIONS env var Oct 17, 2018

nitsakh approved these changes Oct 17, 2018

View reviewed changes

ckerr reviewed Oct 17, 2018

View reviewed changes

codebytere force-pushed the enable-node-options branch 2 times, most recently from 9e9f89a to 7af2eff Compare October 17, 2018 23:37

ckerr approved these changes Oct 18, 2018

View reviewed changes

fix: enable NODE_OPTIONS env var

3bf4072

codebytere force-pushed the enable-node-options branch from 7af2eff to 3bf4072 Compare October 18, 2018 18:05

ckerr approved these changes Oct 18, 2018

View reviewed changes

MarshallOfSound added the target/4-0-x label Oct 18, 2018

MarshallOfSound merged commit a0b9d47 into master Oct 18, 2018

MarshallOfSound deleted the enable-node-options branch October 18, 2018 23:57

trop bot mentioned this pull request Oct 18, 2018

fix: enable use of NODE_OPTIONS env var (backport: 4-0-x) #15259

Merged

trop bot added merged/4-0-x and removed target/4-0-x labels Oct 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: enable use of NODE_OPTIONS env var #15158

fix: enable use of NODE_OPTIONS env var #15158

codebytere commented Oct 14, 2018 •

edited

Loading

alexeykuzmin left a comment •

edited

Loading

alexeykuzmin commented Oct 14, 2018

codebytere commented Oct 15, 2018 •

edited

Loading

nornagon commented Oct 15, 2018

codebytere commented Oct 15, 2018 •

edited

Loading

nornagon commented Oct 15, 2018

deepak1556 commented Oct 15, 2018

codebytere commented Oct 17, 2018 •

edited

Loading

ckerr Oct 17, 2018

ckerr Oct 17, 2018

ckerr Oct 17, 2018 •

edited

Loading

ckerr Oct 17, 2018

deepak1556 commented Oct 17, 2018

ckerr left a comment

ckerr Oct 18, 2018

ckerr Oct 18, 2018

ckerr left a comment

release-clerk bot commented Oct 18, 2018

trop bot commented Oct 18, 2018

fix: enable use of NODE_OPTIONS env var #15158

fix: enable use of NODE_OPTIONS env var #15158

Conversation

codebytere commented Oct 14, 2018 • edited Loading

Description of Change

Checklist

Release Notes

alexeykuzmin left a comment • edited Loading

Choose a reason for hiding this comment

alexeykuzmin commented Oct 14, 2018

codebytere commented Oct 15, 2018 • edited Loading

nornagon commented Oct 15, 2018

codebytere commented Oct 15, 2018 • edited Loading

nornagon commented Oct 15, 2018

deepak1556 commented Oct 15, 2018

codebytere commented Oct 17, 2018 • edited Loading

ckerr Oct 17, 2018

Choose a reason for hiding this comment

ckerr Oct 17, 2018

Choose a reason for hiding this comment

ckerr Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

ckerr Oct 17, 2018

Choose a reason for hiding this comment

deepak1556 commented Oct 17, 2018

ckerr left a comment

Choose a reason for hiding this comment

ckerr Oct 18, 2018

Choose a reason for hiding this comment

ckerr Oct 18, 2018

Choose a reason for hiding this comment

ckerr left a comment

Choose a reason for hiding this comment

release-clerk bot commented Oct 18, 2018

trop bot commented Oct 18, 2018

codebytere commented Oct 14, 2018 •

edited

Loading

alexeykuzmin left a comment •

edited

Loading

codebytere commented Oct 15, 2018 •

edited

Loading

codebytere commented Oct 15, 2018 •

edited

Loading

codebytere commented Oct 17, 2018 •

edited

Loading

ckerr Oct 17, 2018 •

edited

Loading