-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lib: drop Python 2 support in find-python.js #2333
Merged
gengjiawen
merged 15 commits into
nodejs:master
from
DeeDeeG:find-python-only-find-python-3
Mar 26, 2021
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
a9e3219
lib: drop Python 2 support in find-python.js
DeeDeeG 83bf041
test: test find-python against supported Python 3
DeeDeeG 39403a8
Revert "lib: drop "-2" flag for "py.exe" launcher"
DeeDeeG 9c0d980
lib: use "-3" flag with py.exe launcher
DeeDeeG 3c48f70
test: reject Python 2 in test-find-python
DeeDeeG f13747d
lib: accept python '>=3.6.0' in find-python.js
DeeDeeG d1f2aec
lib: use real Python 3 default install paths
DeeDeeG 2562153
test: update Python path for "good guess" test
DeeDeeG d20f929
lib: use correct paths for 32-bit system-wide Python
DeeDeeG 54041d8
lib: add system-wide Python path for 32-bit Windows
DeeDeeG d0bcb5c
lib: construct winDefaultLocations programatically
DeeDeeG a7b43aa
lib: assign localAppData and programFiles from env
DeeDeeG 432447d
lib: fall back to os.userInfo() to get username
DeeDeeG 1711046
test: delete redundant Python 2 check
DeeDeeG ec64f91
test: adjust expected number of tests
DeeDeeG File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of fixed
Users
andProgram 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 toC:\Users\[user]
. The env varLOCALAPPDATA
is stillC:\Users\[user]\AppData\Local
. The env varProgramFiles
is stillC:\Program Files
. Even while this shows asC: > Archivos de programa
in File Explorer, the moment you try to edit it, it changes toC:\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.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And most of all, there is no downside to the change I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go for it. This code is easy to read/ understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that is the Swiss in me but...
Please sort your const declarations in the order that they are used to improve readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
, asnode-gyp
user newer JavaScript features that aren't supported in Node 5. In conclusion: modernnode-gyp
won't run at all on Windows XP or Vista, regardless of this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mfonville thanks for mentioning that. I believe I will need to switch from
process.env.ProgramFiles
toprocess.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-specificProgram 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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I committed this in the PR now.
Program Files
,Program Files (x86)
andAppData\Local
are set from environment variables now.