Skip to content
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

build,win: fix Python detection on localized OS #29423

Closed

Conversation

joaocgreis
Copy link
Member

@joaocgreis joaocgreis commented Sep 3, 2019

This code previously assumed the default value was always printed to the console by reg.exe as (default), but this is not true on localized versions of Windows and can contain spaces.

Fixes: #29417

cc @targos @nodejs/platform-windows

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

This code previously assumed the default value was always printed to
the console by reg.exe as "(default)", but this is not true on
localized versions of Windows and can contain spaces.

Fixes: nodejs#29417
@nodejs-github-bot nodejs-github-bot added install Issues and PRs related to the installers. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. labels Sep 3, 2019
@nodejs-github-bot

This comment has been minimized.

@targos
Copy link
Member

targos commented Sep 3, 2019

CI failed on Windows:

18:05:18 > "C:\Python27\\python.exe" -V   2>&1  | findstr /R "^Python.2.*"  1>NUL 
18:05:18 
18:05:18 > EXIT /B 0 
18:05:18 The system cannot find the batch label specified - found-python2
18:05:18 The system cannot find the batch label specified - done-path-v2
18:05:18 
18:05:18 > if ERRORLEVEL 1 exit /b

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 3, 2019

@joaocgreis
Copy link
Member Author

This was an issue with the CI machine that was used to compile. The git option core.autocrlf should be set to true as is default on Windows, but on that machine was false. I've fixed the machine and added a fixup to enforce that option for find_python.cmd, similar to what already happens for vcbuild.bat. The problem running the cmd file was that goto does not work well when the file has Unix line endings.

@joaocgreis joaocgreis force-pushed the joaocgreis-J93-python-reg-spaces branch from ee23a60 to d372475 Compare September 3, 2019 17:45
@cclauss cclauss added the python PRs and issues that require attention from people who are familiar with Python. label Sep 3, 2019
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Sep 6, 2019

Landed in 7bff962

@Trott Trott closed this Sep 6, 2019
Trott pushed a commit that referenced this pull request Sep 6, 2019
This code previously assumed the default value was always printed to
the console by reg.exe as "(default)", but this is not true on
localized versions of Windows and can contain spaces.

Fixes: #29417

PR-URL: #29423
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: David Carlier <[email protected]>
targos pushed a commit that referenced this pull request Sep 20, 2019
This code previously assumed the default value was always printed to
the console by reg.exe as "(default)", but this is not true on
localized versions of Windows and can contain spaces.

Fixes: #29417

PR-URL: #29423
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
install Issues and PRs related to the installers. python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vcbuild.bat is somewhat broken on Windows
8 participants