Android fixes

[robertchiras]

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• documentation is changed or added
• the commit message follows commit guidelines

Affected core subsystem(s)

build, child_process

Description of change

Small fixes found when running on Android platform.

[bnoordhuis]

The second commit doesn't look obviously correct to me. If the issue is the location of sh, wouldn't a more appropriate fix look like this?

diff --git a/lib/child_process.js b/lib/child_process.js
index 81fb8c1..4fda6e2 100644
--- a/lib/child_process.js
+++ b/lib/child_process.js
@@ -332,7 +332,12 @@ function normalizeSpawnArguments(file /*, args, options*/) {
       args = ['/s', '/c', '"' + command + '"'];
       options.windowsVerbatimArguments = true;
     } else {
-      file = typeof options.shell === 'string' ? options.shell : '/bin/sh';
+      if (typeof options.shell === 'string')
+        file = options.shell;
+      else if (process.platform === 'android')
+        file = '/system/bin/sh';
+      else
+        file = '/bin/sh';
       args = ['-c', command];
     }
   }

[robertchiras]

The second commit is intended to not mask any other exceptions. The exception I met and described is just an example. This is why, I consider it might be a good fix.
Regarding your proposed fix: my way of treading that error, was to correct the test itself, which was calling execSync, by adding shell option.
For example, in test-child-process-emfile:

diff --git a/test/sequential/test-child-process-emfile.js b/test/sequential/test-child-process-emfile.js
index f186db5..961f37c 100644
--- a/test/sequential/test-child-process-emfile.js
+++ b/test/sequential/test-child-process-emfile.js
@@ -9,13 +9,19 @@ if (common.isWindows) {
   return;
 }

-const ulimit = Number(child_process.execSync('ulimit -Hn'));
+var sh = '/bin/sh'
+var opts = {}
+if (process.platform === 'android') {
+  sh = '/system/bin/sh'
+  opts = {shell: sh}
+}
+const ulimit = Number(child_process.execSync('ulimit -Hn', opts));
 if (ulimit > 64 || Number.isNaN(ulimit)) {
   // Sorry about this nonsense. It can be replaced if
   // https://github.com/nodejs/node-v0.x-archive/pull/2143#issuecomment-2847886
   // ever happens.
   const result = child_process.spawnSync(
-    '/bin/sh',
+    sh,
     ['-c', `ulimit -n 64 && '${process.execPath}' '${__filename}'`]
   );
   assert.strictEqual(result.stdout.toString(), '');

But, I think that your proposed fix is better, since we won't have to handle this situation in every test using execSync.

[bnoordhuis]

Okay, I understand now. Can you add a regression test to the second commit that exercises the stderr-is-falsy code path? I filed #6745 for the /bin/sh patch.

[eljefedelrodeodeljefe]

ping @robertchiras do you find some time to work on this? #6745 landed. Thanks!

[robertchiras]

Hi @eljefedelrodeodeljefe
Yes, I did not forgot about this. But, I updated node to latest version and I found out it does not compile anymore for Android. I am preparing a new patch and I will separate the Android build issues from the child_process issue into different pull requests.

[eljefedelrodeodeljefe]

Very grateful @robertchiras. Looking forward to it. I am collecting similar issues across the repo. After we land the stalled stuff I am gonna have a look at building latest too. Thanks again.