Update wrong catch block

[liviarett]

Checklist

• npm install && npm test passes
• tests are included
• documentation is changed or added
• commit message follows commit guidelines

Description of change

[cclauss]

@DeeDeeG Your review please?

https://stackoverflow.com/questions/3335376/why-do-good-programmers-sometimes-silently-swallow-exceptions

[rvagg]

Yeah, sorry, this is intentional (IIRC), there's some OS' where this fails, I can't remember the details, it was some trimmed-down Linux, I'm sure git blame would point the way to the PR that introduced the quiet fail.

Is this causing problems for you?

[liviarett]

hmm I had a few other issues that were causing me to get to this point anyway, but this is disguising the error, as it gives me

gyp ERR! UNCAUGHT EXCEPTION
gyp ERR! stack /Users/liviarett/.nvm/versions/node/v8.17.0/lib/node_modules/node-gyp/lib/find-python.js:44
gyp ERR! stack   } catch {}
gyp ERR! stack           ^
gyp ERR! stack
gyp ERR! stack SyntaxError: Unexpected token {
gyp ERR! stack     at createScript (vm.js:80:10)
gyp ERR! stack     at Object.runInThisContext (vm.js:139:10)
gyp ERR! stack     at Module._compile (module.js:617:28)
gyp ERR! stack     at Object.Module._extensions..js (module.js:664:10)
gyp ERR! stack     at Module.load (module.js:566:32)
gyp ERR! stack     at tryModuleLoad (module.js:506:12)
gyp ERR! stack     at Function.Module._load (module.js:498:3)
gyp ERR! stack     at Module.require (module.js:597:17)
gyp ERR! stack     at require (internal/module.js:11:18)
gyp ERR! stack     at Object.<anonymous> (/Users/liviarett/.nvm/versions/node/v8.17.0/lib/node_modules/node-gyp/lib/configure.js:11:18)

but not a big problem :)

[rvagg]

ookkaayy ... my memory is failing me on this one, I see this particular change was introduced @ fca4795 and that I must be thinking of another silent catch that we introduced eons ago.. So this is pretty new, and a legit parse error.

👍 so let's fix the parse error you're getting, but not force the throw.

[rvagg]

ahha! I knew I wasn't going that senile: #1835 but it's in install.js, while this same code is now in find-python.js which was just recently refactored.

[liviarett]

ah alright! nice, so we can at least fix the unexpected token thing

[DeeDeeG]

Why I used catch {} rather than catch (e) {}

@rvagg #2441 (comment)

The catch {} syntax is valid on our supported versions of Node (10.12 and newer). I honestly hadn't anticipated this would ever run on Node older than that, but in hindsight that's a little optimistic.

The catch {} I added in #2375 is admittedly a syntax error on older Node. Older node wants the error to be bound to some variable (usually e) to be passed into the catch block: catch (e) {} even if you don't intend to use the error for anything within the block.

catch {} without binding the error is an EcmaScript 2019 thing. MDN says this is a supported as of Node 10/Chrome 66, and the V8 blog says this is part of V8 6.6 (https://v8.dev/blog/v8-release-66#optional-catch-binding). See also: the Node 10.0.0 Changelog ("V8 has been updated to 6.6. [9daebb48d6]"). I tested, and catch {} works on Node 10, but not Node 9, 8, or older...)

Why I used an empty catch{} ("swallowing the error")

@cclauss #2441 (comment)

I tried logging the error (#2375 (comment) --> implementation), but that added complexity and size of code seemed a bit overkill, given we intend to fail gracefully and just move on. It is hard to properly add logging at file-level scope (outside of PythonFinder) without a major refactor, and I found refactoring this into PythonFinder tricky or impossible, since this data needs to be computed (or computable on-demand via a reachable/defined function) by the time winDefaultLocations is assigned in PythonFinder... (I am self-taught with JS, and PythonFinder honestly confuses me a bit.)

Adding the error binding catch (e) {} doesn't hurt

I'm alright with it being "fixed" for older Node versions. At the cost of just three added characters, we allow Node older than 10 to try to do whatever parts of node-gyp stil work on Node older than 10. I note that the latest node-gyp does not work perfectly on Node older than 10.12.0, though: #2152, so users should try to use Node 10.12 or newer if they can. Node 10 is now end-of-life, by the way: https://nodejs.org/en/about/releases/... So Node 12 is the minimum still officially supported version of Node folks should use, again if they can.

[DeeDeeG]

👍 Avoids a syntax error on older Node (Node 9.x or older).

(Note: Those versions of Node are technically not supported by the latest node-gyp, because of #2123 --> #2152. But I think not causing a syntax error in the program for older Node is still an improvement.)

Due to #2152, users should really be on Node 10.12 or newer when using the latest node-gyp, and ideally they would be on the latest Node 12.x or 14.x or 16.x. But I don't think fixing this syntax error for older Node (at the cost of three characters of JS) hurts anything.

[rvagg]

👍 thanks @DeeDeeG, it's good to lean on the conservative side for node-gyp since it's in such wide usage. When people show up using older versions of Node.js (for whatever reason - sometimes they can't change) and well tell them that their old version of node-gyp doesn't work for their use-case, it's not fun to tell them they can't get the newer one cause their Node.js is old. So where the change is painless, like this one, let's opt for the conservative choice.

[DeeDeeG]

Update: I found out that trying to use node-gyp 8 on older Node (Node < 10) is still rather busted, due to #2220.

Here's the details and a rather hacky workaround...

#2220 broke node-gyp install (and, by extension, broke node-gyp rebuild) on Node older than version 10, by relying on the stream.pipeline Node API function that was only added in Node v10.0.0. And by switching a bunch of fs.x to fsPromises.x. (The fsPromises API was also added in Node 10.0.0).

Workaround if you really need to run the latest node-gyp on Node 8.x would be:

Caveat/warning: All this only applies to node-gyp@8.1.0. Only tested somewhat working by running node-gyp rebuild via npm rebuild... Using npm 6.13.4 and Node 8.17.0. Only tested on Linux. Any other combination may not work, and none of this is actually officially supported by node-gyp or npm or Node, etc. Try at your own risk.

• Install older Node's development headers with an older node-gyp. (For example, node-gyp 6 works on Node 8.)
  • (With the older Node version, run npx node-gyp@6 install)
• Then, go in with a text editor and manually edit the copy of node-gyp you want to run; Edit node-gyp's lib/find-python.js file like this PR does; update the catch {} block in lib/find-python.js to pass in the error: catch (e) {}.
  • (Patch the file here.)
• Then in lib/install.js...
  • Go in with a text editor and manually replace const streamPipeline = util.promisify(stream.pipeline) near the top of the file with an empty function: const streamPipeline = function () {}.
    • (here.)
  • And replace fs.promises.stat() with fs.statSync(), and replace fs.promises.readFile() with fs.readFileSync(), and delete the await immediately preceding these two fs API calls, since we're using basic synchronous code now, not Promises.
    • (Only needed in two places. here and here.)

See also https://github.com/nodejs/node-gyp/blob/master/docs/Updating-npm-bundled-node-gyp.md for the likely locations of node-gyp when bundled with npm.

I would personally try with node-gyp 7 or 6 instead, if I needed to use node-gyp on older Node. Preferable is of course to use a supported version of Node, such as 12, 14, or 16, if that is possible.