child_process: allow `URL` instances as paths to executables #49031

LiviaMedeiros · 2023-08-05T22:27:05Z

Allow URL instances to be passed as file parameters and execPath options. For example:

import { execFile } from 'node:child_process';

execFile(
  new URL('../../bin/executable', import.meta.url),
  ['argv1'],
  { cwd: new URL('../dir/subdir', import.meta.url) },
  () => { ... }
);

mscdex · 2023-08-05T22:45:01Z

test/common/index.js

+  [new URL('file:///C:/Windows/system32/cmd.exe'), ['/d', '/c', 'cd']] :
+  [new URL('file:///bin/pwd'), []];


Why these changes? Are we sure these absolute paths are safe assumptions?

Not 100% sure and I'd like to see if it's not the case on any platform covered by CI first.

Then it will most likely be moved to separate test-child-process-urlfile.mjs test.

If there is a concise&robust way to get absolute url or absolute path to pwd or any other convenient good-for-test executable, or if we can create one in fixtures, I'd like to know.

I would guess the "where"/"which" commands should generally be available, even outside of CI since tests should work locally too.

github-actions · 2023-08-06T04:46:44Z

Failed to start CI

- Validating Jenkins credentials
✘  Jenkins credentials invalid

https://github.com/nodejs/node/actions/runs/5774680936

anonrig · 2023-08-06T15:55:25Z

doc/api/child_process.md

 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/49031
+    description: The `command` parameter can be a WHATWG `URL` object using


The word object hints towards an actual javascript object with URL class properties, but there isn't any test to check towards it.

Suggested change

description: The `command` parameter can be a WHATWG `URL` object using

description: The `command` parameter can be a WHATWG `URL` instance using

isURL() is duck-typed:

node/lib/internal/url.js

Lines 761 to 763 in d150316

function isURL(self) {

return Boolean(self?.href && self.protocol && self.auth === undefined && self.path === undefined);

}

so it probably makes sense? The same term is used in other entries here as well.

anonrig · 2023-08-06T15:59:11Z

test/parallel/test-child-process-urlfile.mjs

+}
+
+{
+  const stdout = cp.spawnSync(...pwdCommandAndOptions).stdout;


Can you also add a test where the legacy url class is used? We need to make sure it does exactly what we want it to be.

anonrig · 2023-08-06T16:00:08Z

test/parallel/test-child-process-urlfile.mjs

+const pwdFullPath = `${cp.execSync(whichCommand)}`.trim();
+const pwdUrl = pathToFileURL(pwdFullPath);
+const pwdCommandAndOptions = isWindows ?
+  [pwdUrl, ['/d', '/c', 'cd'], { cwd: pathToFileURL(cwd) }] :


Can you also add a test where javascript object representation of the URL is used?

anonrig · 2023-08-06T16:01:56Z

lib/child_process.js

 function normalizeSpawnArguments(file, args, options) {
-  validateString(file, 'file');
-  validateArgumentNullCheck(file, 'file');
+  file = getValidatedPath(file, 'file');


validatePath inside getValidatedPath also accepts buffers. Can you add a test for it as well?

Nice catch! I guess it should be restricted to string or stringified URL, at least for now.

anonrig · 2023-08-06T16:04:38Z

cc @nodejs/url

anonrig · 2023-08-06T21:37:40Z

test/parallel/test-child-process-urlfile.mjs

+
+  throws(
+    () => cp.execFileSync(...pwdCommandAndOptions),
+    { code: 'ERR_INVALID_ARG_TYPE' },


Can you be specific, or at least add comment about what this line is testing? It is really cryptic atm.

Added tests for specific errors, added brief comments.

nodejs-github-bot · 2023-08-10T17:32:17Z

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

nodejs-github-bot · 2023-09-07T12:44:53Z

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

nodejs-github-bot · 2023-09-07T18:42:42Z

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

nodejs-github-bot · 2023-09-08T02:10:57Z

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

nodejs-github-bot · 2023-09-08T04:47:27Z

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

nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. labels Aug 5, 2023

mscdex reviewed Aug 5, 2023

View reviewed changes

LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 6, 2023

github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Aug 6, 2023

anonrig reviewed Aug 6, 2023

View reviewed changes

aduh95 added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Aug 8, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2023

This comment was marked as outdated.

Sign in to view

LiviaMedeiros added 4 commits August 11, 2023 01:30

child_process: allow URL instances as paths to executables

29fc263

squash: add test

79ae91a

squash: strict validation, test for bad input

d7aaae1

squash: more tests

71a2fef

LiviaMedeiros force-pushed the childprocess-file-url branch from 7ca5ecf to 71a2fef Compare August 10, 2023 17:31

squash: skip non-empty host test on Windows

d25509f

LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2023

github-actions bot mentioned this pull request Sep 8, 2023

CI Reliability 2023-09-08 nodejs/reliability#659

Open

26 tasks

LiviaMedeiros added the review wanted PRs that need reviews. label Sep 8, 2023

github-actions bot mentioned this pull request Sep 9, 2023

CI Reliability 2023-09-09 nodejs/reliability#660

Open

24 tasks

This was referenced Sep 10, 2023

CI Reliability 2023-09-10 nodejs/reliability#661

Open

CI Reliability 2023-09-11 nodejs/reliability#662

Open

		[new URL('file:///C:/Windows/system32/cmd.exe'), ['/d', '/c', 'cd']] :
		[new URL('file:///bin/pwd'), []];

	description: The `command` parameter can be a WHATWG `URL` object using
	description: The `command` parameter can be a WHATWG `URL` instance using

	function isURL(self) {
	return Boolean(self?.href && self.protocol && self.auth === undefined && self.path === undefined);
	}

Uh oh!

child_process: allow URL instances as paths to executables #49031

Are you sure you want to change the base?

child_process: allow URL instances as paths to executables #49031

Conversation

LiviaMedeiros commented Aug 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anonrig commented Aug 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Aug 10, 2023

Uh oh!

nodejs-github-bot commented Sep 7, 2023

Uh oh!

nodejs-github-bot commented Sep 7, 2023

Uh oh!

nodejs-github-bot commented Sep 8, 2023

Uh oh!

nodejs-github-bot commented Sep 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

child_process: allow `URL` instances as paths to executables #49031

child_process: allow `URL` instances as paths to executables #49031