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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

joaocgreis
Copy link
Member

@joaocgreis joaocgreis commented Sep 3, 2019 •
edited
Loading

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

@joaocgreis
build,win: fix Python detection on localized OS
6c93e5b 
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
@joaocgreis joaocgreis mentioned this pull request Sep 3, 2019
Closed
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@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 •
edited by joaocgreis
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/25338/ ✔️

@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
@cclauss cclauss mentioned this pull request Sep 3, 2019
Closed
63 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/25373/

@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
@joaocgreis @Trott
build,win: fix Python detection on localized OS
7bff962 
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 <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
targos pushed a commit that referenced this pull request Sep 20, 2019
@joaocgreis @targos
build,win: fix Python detection on localized OS
42fd139 
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 <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
Merged
This was referenced Sep 25, 2019
Closed
Closed
Closed
Closed
Closed
@beevelop beevelop mentioned this pull request Sep 26, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JungMinu JungMinu JungMinu approved these changes

@devnexen devnexen devnexen approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

vcbuild.bat is somewhat broken on Windows
8 participants
@joaocgreis @nodejs-github-bot @targos @Trott @jasnell @devnexen @JungMinu @cclauss