lib: drop Python 2 support in find-python.js

[DeeDeeG]

Checklist

• npm install && npm test passes
• tests are included (I updated existing tests)
• documentation is changed or added
• commit message follows commit guidelines

Description of change

Follow-up to #2300 and #2318.

(Now that gyp-next no-longer supports Python 2, node-gyp should stop finding and trying to use Python 2 to run gyp-next with.)

This Pull Request:

• Updates the accepted versions of Python in lib/find-python.js from '2.7.x || >=3.5.0' to just '>=3.6.0'
• Removes checks attempting to find Python 2 (on all platforms)
• Updates the fallback paths (default Python install paths) to check on Windows
• Uses the -3 flag with the py.exe launcher (on Windows), so that it looks for/executes only a Python 3 executable
• Updates the tests to reflect the above changes

[cclauss]

LGTM! Thanks.

[mfonville]

Instead of fixed Users and Program Files shouldn't this be with the environment variables %ProgramFiles%, %ProgramFiles(x86)%, %ProgramW6432% and %LocalAppData%?

Because these folders can be set to be on other disks than the system drive, and avoids potential localization issues in the path.

[DeeDeeG]

@mfonville Thanks for the suggestion. I tried your suggestion on another branch: DeeDeeG@e57fdcc

It does work. It's also a bit more involved. After some quick research and testing, I am somewhat doubtful whether there are circumstances where the paths actually change based on localization or user customization. See my thoughts below. If you have examples or more information about when/if the paths are actually different, please let me know.

At the moment I am inclined to believe this is not necessary. But I would not mind being proved wrong. Thanks.

---

More thoughts on this (click to expand):

I wish I knew in what circumstances the "Program Files" or "AppData" folders are localized or moved, if there are any such circumstances. I do not think the paths are truly localized on-disk, nor movable, in Windows 10. It's unclear to me if they've ever been truly localized on-disk, or movable in previous Windows releases.

(Paths were slightly different in Windows XP, but Windows XP is not compatible with the latest node-gyp. Python 3.4 is the last version supporting Windows XP. node-gyp master branch now requires Python 3.6 and up, so no XP support anymore. And only Windows 10 is in "mainstream" support by Microsoft now.)

Regarding custom locations for Program Files or AppData: Is anyone here aware of a proper way to move these folders? (I am aware that folders such as Documents and Videos can be easily moved to another drive. But can "Program Files", user home folders, or "AppData" be moved off of the system drive? I have looked this up online, and the overall impression I am getting is that there is no official way to move these, and that moving them may break things.)

Regarding localization, or Windows in languages other than English: I don't see the paths being actually localized on the disk. (On Windows 7 or 10 in Spanish, I see it presented visually in the File Explorer as "C: > Usuarios > [user]", but the moment you try to edit the path in the address bar, it changes to C:\Users\[user]. The env var LOCALAPPDATA is still C:\Users\[user]\AppData\Local. The env var ProgramFiles is still C:\Program Files. Even while this shows as C: > Archivos de programa in File Explorer, the moment you try to edit it, it changes to C:\Program Files. I believe the "non-localized" versions are the true paths, and the localizations are superficial. At least this is the case in my testing on the latest Windows 10, and on an install made from an old Windows 7 iso.)

[mfonville]

If XP is out of the picture, then I do think localization is probably indeed not an issue anymore. Windows 10 does this nicely.

But about custom paths, to be honest I am not working with Windows daily anymore (switched to Linux many years ago). But when a couple of years ago I did do some volunteering with managing some Windows systems, we could set custom locations for these paths through policies. And also I have seen customized paths when computers were upgraded from a HDD to a SSD, where these paths were customized to optimize performance.
I am not sure how Windows 10 handles it these days, but imho those environment variables are more reliable than any hardcoded path. So better to use the variables :-)

[DeeDeeG]

Thanks for that explanation. I also found this official Microsoft document about moved user folders: https://docs.microsoft.com/en-us/troubleshoot/windows-server/user-profiles-and-logon/relocation-of-users-and-programdata-directories

So apparently user directories can be moved to a custom location. (Maybe this applies only to Windows Server? I'd say node-gyp is not any less important for Windows Server users. It still seems like a bit of an obscure feature and is discouraged in the docs I linked.)

I'm a little more inclined to do this (DeeDeeG@e57fdcc) now. Truth be told, it's not much more complex. Given even a mild justification for end-user benefit, I'd want to do it.

NOTE TO MAINTAINERS/REVIEWERS: Please let me know if you'd like me to add this (DeeDeeG@e57fdcc) to the PR. Thank you.

[mfonville]

And most of all, there is no downside to the change I think.

[cclauss]

Go for it. This code is easy to read/ understand.

[cclauss]

I know that is the Swiss in me but...

Please sort your const declarations in the order that they are used to improve readability.

[DeeDeeG]

More research: We definitely don't have to worry about Windows XP or Vista.

Node v6 refuses to run on anything older than Windows 7. Node v5 is the newest Node that will run on Windows Vista. Node v5 cannot even run the latest node-gyp, as node-gyp user newer JavaScript features that aren't supported in Node 5. In conclusion: modern node-gyp won't run at all on Windows XP or Vista, regardless of this PR.

[DeeDeeG]

environment variables [. . .] %ProgramW6432% [. . .]

@mfonville thanks for mentioning that. I believe I will need to switch from process.env.ProgramFiles to process.env.ProgramW6432 to get the "not (x86)" Program Files.

(Note: this is important when running 32-bit Node on 64-bit Windows, which is common when building 32-bit code on a 64-bit host, such as in CI.)

It appears we have to use process.env.ProgramW6432 to get the not-architecture-specific Program Files location. From 32-bit Node on a 64-bit host, process.env.ProgramFiles returns [driveLetter]:\Program Files (x86). As such, ProgramW6432 is the only easy way to get [driveLetter]:\Program Files from the environment across all combinations of host arch/node arch.

(Official documentation says that the env var ProgramW6432 only exists as of Windows 7 or newer, but that's okay. node-gyp doesn't even run on Windows Vista or older.)

See:

• https://docs.microsoft.com/en-us/windows/win32/winprog64/wow64-implementation-details?redirectedfrom=MSDN#environment-variables
• https://serverfault.com/questions/413320/programfiles-differences-on-64bit-windows

[DeeDeeG]

Okay, I committed this in the PR now. Program Files, Program Files (x86) and AppData\Local are set from environment variables now.

[DeeDeeG]

I think this should fall back to assigning some string to username. Even something not very useful like '' or 'User'.

(It's possible both process.env.USERNAME and process.env.USER are undefined. Having a not very useful path to check is better than seeing Uncaught ReferenceError: username is not defined.)

[cclauss]

I actually like the error message better than unsuccessfully trying to sweep the problem under the carpet. The message makes it clear what the problem is.

Please do not worry about the Python 4 case. We will cross that bridge later.

[cclauss]

Thinking a bit more...

If username is undefined and we can find a globally installed acceptable Python then we should use that Python without complaint. If we fail to find an acceptable Python and username is undefined then we should raise the error.

[cjihrig]

You can also get the username via os.userInfo().

[DeeDeeG]

Thanks for that suggestion @cjihrig. os.userInfo() should always return a username string. I committed that as a fallback. I think this can reasonably be expected to prevent Uncaught ReferenceError: username is not defined.

[DeeDeeG]

Note: There was apparently no risk of Uncaught ReferenceError after all.

If you write .someNotDefinedThing off of an object or array, you get undefined as the result, but no ReferenceError. Since process.env returns an object, doing process.env.THIS_IS_NOT_DEFINED does not produce a ReferenceError.

For example:

console.log(`Here is my undefined property: ${{}.some_undefined_property}`)

Outputs the string:

Here is my undefined property: undefined

So we would construct a Python path resembling this: C:\\Users\\undefined\\AppData\\Local\\Programs\\Python\\Python39\\python.exe

(Note the Users\\undefined bit.)

I'm happy with the os.userInfo() fallback, regardless. It makes it pretty much guaranteed we can find the username. This is just an interesting finding from my tests.

[beltran-rubo]

Related to this, still an issue when the username is not defined, only the user ID e.g. that is common in 'non-root' containers for OpenShift https://docs.openshift.com/container-platform/3.3/creating_images/guidelines.html#openshift-container-platform-specific-guidelines.

I have no name!@52545ce360b8$ node-gyp --help
os.js:272
    throw new ERR_SYSTEM_ERROR(ctx);
    ^

SystemError [ERR_SYSTEM_ERROR]: A system error occurred: uv_os_get_passwd returned ENOENT (no such file or directory)
    at Object.userInfo (os.js:272:11)
    at Object.<anonymous> (/opt/bitnami/node/lib/node_modules/node-gyp/lib/find-python.js:11:76)
    at Module._compile (internal/modules/cjs/loader.js:999:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
    at Module.load (internal/modules/cjs/loader.js:863:32)
    at Function.Module._load (internal/modules/cjs/loader.js:708:14)
    at Module.require (internal/modules/cjs/loader.js:887:19)
    at require (internal/modules/cjs/helpers.js:74:18)
    at Object.<anonymous> (/opt/bitnami/node/lib/node_modules/node-gyp/lib/configure.js:11:18)
    at Module._compile (internal/modules/cjs/loader.js:999:30) {
  code: 'ERR_SYSTEM_ERROR',
  info: {
    errno: -2,
    code: 'ENOENT',
    message: 'no such file or directory',
    syscall: 'uv_os_get_passwd'
  },
  errno: [Getter/Setter],
  syscall: [Getter/Setter]
}

Python is in the path so a good option could be to add a fallback or not return an error for the line

const username = process.env.USERNAME || process.env.USER || require('os').userInfo().username

[DeeDeeG]

I think the py.exe launcher works really well. I'd like to suggest it be tried before all these winDefaultLocations entries. I'm not sure if that's out-of-scope for this PR though.

(I noticed while testing this pull request that the py.exe launcher is run after checking all these potential paths from winDefaultLocations.)

(Again, not sure if it would be part of this PR or not, but here's an example implementation: DeeDeeG@c39ad57).

[cclauss]

@cjihrig @mfonville Are you folks satisfied with the current state of this PR? There is nice work here and it would be good to have more PR approvals besides @gengjiawen and myself.

[mfonville]

I am happy with its current state. You could still consider @DeeDeeG's last suggestion to try py.exe before all the generated entries. That would probably be the logical thing to do.

[cclauss]

@mfonville

1. https://github.com/nodejs/node-gyp/pull/2333/files
2. Review changes
3. Approve
4. Submit review

[DeeDeeG]

Reminder: I think it might be neat to prioritize trying the py.exe launcher, before checking all the default install paths, on Windows.

I have an implementation of this ready to go. (DeeDeeG@c39ad57) Please let me know if this is preferred. And/or if you want that, but as another PR.

[rvagg]

I'm happy to include this in v8.0.0 but there's been some additional commits since @gengjiawen and @cclauss approved, would either of you mind taking a look over the current state?
I'll squash it into a single commit for merging, unless anyone thinks there's separate things going on here worth individuating.

[gengjiawen]

@cclauss Still looks good to you ?

[cclauss]

This PR already has my approval.