child_process: simplify spawn argument parsing #27854

cjihrig · 2019-05-24T15:06:25Z

This commit simplifies the object returned by normalizeSpawnArguments(). In general, this simplifies the code by removing options vs. opts logic.

This does impact monkey patching, as illustrated by the changes in tests. For that reason, I'm conservatively labeling as semver-major. If anyone disagrees, feel free to remove the label.

Checklist

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

nodejs-github-bot · 2019-05-24T15:06:27Z

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3613

himself65 · 2019-05-24T15:19:47Z

test/parallel/test-child-process-spawnsync-shell.js

+      assert.strictEqual(opts.shell, shell);
+      assert.strictEqual(opts.file, opts.file);
+      assert.deepStrictEqual(opts.args, opts.args);
+      assert.strictEqual(opts.windowsHide, false);


Should opts.windowsHide only exist on Windows system?

It's only meaningful on Windows, but it's been set on every spawn() call since it was introduced, so I think it's fine.

nodejs-github-bot · 2019-05-26T14:39:03Z

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

nodejs-github-bot · 2019-05-26T15:40:32Z

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

nodejs-github-bot · 2019-05-27T04:10:36Z

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

nodejs-github-bot · 2019-05-27T14:40:03Z

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

nodejs-github-bot · 2019-05-27T15:08:13Z

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

nodejs-github-bot · 2019-05-27T17:21:54Z

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

sam-github · 2019-05-27T17:39:48Z

@cjihrig Sorry, I cancelled the AIX build, it was running on a machine which always fails.

cjihrig · 2019-05-27T17:42:16Z

@sam-github that's fine. Is there something else I can do? AIX has been the only failure, and I'm pretty sure it's unrelated. I've seen similar failures on AIX for other PRs.

nodejs-github-bot · 2019-05-27T17:42:58Z

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

EDIT(cjihrig): CI was finally green.

sam-github · 2019-05-27T17:47:02Z

I resumed it.

We are still looking into what's going on. https://ci.nodejs.org/job/node-test-commit-aix/ shows all builds failing, and they all seem to fail on the same set of history and repl tests. Disk space seems fine, there don't seem to be left over processes. I'm baffled. @mhdawson suggested a jenkins agent restart. I'll see if it changed anything.

This commit simplifies the object returned by normalizeSpawnArguments(). This does impact monkey patching, as illustrated by the changes in tests. PR-URL: nodejs#27854 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

BridgeAR · 2019-06-17T17:15:54Z

@cjihrig looking at the code it seems to me this could be partially backported to v12.x. Would you be so kind and open a backport for this where you cherry-pick this commit and add a compatibility patch on top that removes the actual breaking change?

BridgeAR · 2019-08-06T13:27:16Z

Ping @cjihrig

Notable changes: - **assert**: - do not repeat .throws() code (Ruben Bridgewater) [#28263](#28263) - wrap validation function errors (Ruben Bridgewater) [#28263](#28263) - fix generatedMessage property (Ruben Bridgewater) [#28263](#28263) - improve class instance errors (Ruben Bridgewater) [#28263](#28263) - **benchmark**: - use test/common/tmpdir consistently (João Reis) [#28858](#28858) - **build**: - make full-icu the default for releases (Richard Lau) [#29887](#29887) - update minimum Xcode version for macOS (Michael Dawson) [#29622](#29622) - **child_process**: - runtime deprecate \_channel (cjihrig) [#27949](#27949) - simplify spawn argument parsing (cjihrig) [#27854](#27854) - **console**: - display timeEnd with suitable time unit (Xavier Stouder) [#29251](#29251) - **deps**: - patch V8 to 7.8.279.14 (Myles Borins) [#29694](#29694) - patch V8 to 7.8.279.12 (Myles Borins) [#29694](#29694) - patch V8 to 7.8.279.10 (Myles Borins) [#29694](#29694) - update V8's postmortem script (cjihrig) [#29694](#29694) - V8: cherry-pick 716875d (Myles Borins) [#29694](#29694) - update V8 to 7.8.279.9 (Myles Borins) [#29694](#29694) - V8: cherry-pick b33af60 (Michaël Zasso) [#28016](#28016) - update V8 to 7.6.303.28 (Michaël Zasso) [#28016](#28016) - **domain**: - error handler runs outside of its domain (Julien Gilli) [#26211](#26211) - **fs**: - make FSWatcher.start private (Lucas Holmquist) [#29905](#29905) - add runtime deprecate for file stream open() (Robert Nagy) [#29061](#29061) - allow int64 offset in fs.write/writeSync/fd.write (Zach Bjornson) [#26572](#26572) - use IsSafeJsInt instead of IsNumber for ftruncate (Zach Bjornson) [#26572](#26572) - allow int64 offset in fs.read/readSync/fd.read (Zach Bjornson) [#26572](#26572) - close file descriptor of promisified truncate (João Reis) [#28858](#28858) - **http**: - do not emit end after aborted (Robert Nagy) [#27984](#27984) - don't emit 'data' after 'error' (Robert Nagy) [#28711](#28711) - remove legacy parser (Anna Henningsen) [#29589](#29589) - throw if 'host' agent header is not a string value (Giorgos Ntemiris) [#29568](#29568) - replace superfluous connection property with getter/setter (Robert Nagy) [#29015](#29015) - fix test where aborted should not be emitted (Robert Nagy) [#20077](#20077) - remove default 'timeout' listener on upgrade (Luigi Pinca) [#26030](#26030) - **http, http2**: - remove default server timeout (Ali Ijaz Sheikh) [#27558](#27558) - **http2**: - remove security revert flags (Anna Henningsen) [#29141](#29141) - remove callback-based padding (Anna Henningsen) [#29144](#29144) - **lib**: - rename validateInteger to validateSafeInteger (Zach Bjornson) [#26572](#26572) - correct error.errno to always be numeric (Joyee Cheung) [#28140](#28140) - no need to strip BOM or shebang for scripts (Refael Ackermann) [#27375](#27375) - rework logic of stripping BOM+Shebang from commonjs (Gus Caplan) [#27768](#27768) - **module**: - runtime deprecate createRequireFromPath() (cjihrig) [#27951](#27951) - **readline**: - error on falsy values for callback (Sam Roberts) [#28109](#28109) - **repl**: - close file descriptor of history file (João Reis) [#28858](#28858) - **src**: - bring 425 status code name into accordance with RFC 8470 (Sergei Osipov) [#29880](#29880) - update NODE\_MODULE\_VERSION to 79 (Myles Borins) [#29694](#29694) - update NODE\_MODULE\_VERSION to 78 (Michaël Zasso) [#28918](#28918) - add error codes to errors thrown in C++ (Yaniv Friedensohn) [#27700](#27700) - use non-deprecated overload of V8::SetFlagsFromString (Michaël Zasso) [#28016](#28016) - update NODE\_MODULE\_VERSION to 77 (Michaël Zasso) [#28016](#28016) - update NODE\_MODULE\_VERSION to 74 (Refael Ackermann) [#27375](#27375) - make process.env.TZ setter clear tz cache (Ben Noordhuis) [#20026](#20026) - enable V8's WASM trap handlers (Gus Caplan) [#27246](#27246) - **stream**: - throw unhandled error for readable with autoDestroy (Robert Nagy) [#29806](#29806) - always invoke callback before emitting error (Robert Nagy) [#29293](#29293) - invoke callback before emitting error always (Robert Nagy) [#29293](#29293) - do not flush destroyed writable (Robert Nagy) [#29028](#29028) - don't emit finish on error (Robert Nagy) [#28979](#28979) - disallow stream methods on finished stream (Robert Nagy) [#28687](#28687) - do not emit after 'error' (Robert Nagy) [#28708](#28708) - fix destroy() behavior (Robert Nagy) [#29058](#29058) - simplify `.pipe()` and `.unpipe()` in Readable (Weijia Wang) [#28583](#28583) - **tools**: - patch V8 to run on older XCode versions (Ujjwal Sharma) [#29694](#29694) - update V8 gypfiles (Michaël Zasso) [#29694](#29694) - support full-icu by default (Steven R. Loomis) [#29522](#29522) - **util**: validate formatWithOptions inspectOptions (Ruben Bridgewater) [#29824](#29824) PR-URL: #29504

cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 24, 2019

nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label May 24, 2019

himself65 reviewed May 24, 2019

View reviewed changes

BridgeAR approved these changes May 25, 2019

View reviewed changes

targos approved these changes May 25, 2019

View reviewed changes

addaleax approved these changes May 26, 2019

View reviewed changes

Trott approved these changes May 27, 2019

View reviewed changes

cjihrig force-pushed the spawn-args branch from dec9d01 to a21712e Compare May 27, 2019 15:07

cjihrig force-pushed the spawn-args branch from a21712e to 4f9cd27 Compare May 27, 2019 18:12

cjihrig merged commit 4f9cd27 into nodejs:master May 27, 2019

cjihrig deleted the spawn-args branch May 27, 2019 18:14

targos added the dont-land-on-v12.x label Jun 3, 2019

BridgeAR added backport-requested-v12.x and removed dont-land-on-v12.x labels Jun 17, 2019

BethGriggs mentioned this pull request Sep 9, 2019

2019-10-22, Version 13.0.0 (Current) #29504

Merged

BridgeAR removed the backport-requested-v12.x label Sep 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

child_process: simplify spawn argument parsing #27854

child_process: simplify spawn argument parsing #27854

cjihrig commented May 24, 2019

nodejs-github-bot commented May 24, 2019

himself65 May 24, 2019

cjihrig May 24, 2019

nodejs-github-bot commented May 26, 2019

nodejs-github-bot commented May 26, 2019

nodejs-github-bot commented May 27, 2019

nodejs-github-bot commented May 27, 2019

nodejs-github-bot commented May 27, 2019

nodejs-github-bot commented May 27, 2019

sam-github commented May 27, 2019

cjihrig commented May 27, 2019

nodejs-github-bot commented May 27, 2019 •

edited by cjihrig

Loading

sam-github commented May 27, 2019

BridgeAR commented Jun 17, 2019

BridgeAR commented Aug 6, 2019

child_process: simplify spawn argument parsing #27854

child_process: simplify spawn argument parsing #27854

Conversation

cjihrig commented May 24, 2019

Checklist

nodejs-github-bot commented May 24, 2019

himself65 May 24, 2019

Choose a reason for hiding this comment

cjihrig May 24, 2019

Choose a reason for hiding this comment

nodejs-github-bot commented May 26, 2019

nodejs-github-bot commented May 26, 2019

nodejs-github-bot commented May 27, 2019

nodejs-github-bot commented May 27, 2019

nodejs-github-bot commented May 27, 2019

nodejs-github-bot commented May 27, 2019

sam-github commented May 27, 2019

cjihrig commented May 27, 2019

nodejs-github-bot commented May 27, 2019 • edited by cjihrig Loading

sam-github commented May 27, 2019

BridgeAR commented Jun 17, 2019

BridgeAR commented Aug 6, 2019

nodejs-github-bot commented May 27, 2019 •

edited by cjihrig

Loading