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: replace run-python subroutine with single find_python call #18621

Closed
wants to merge 1 commit into from
Closed

build,win: replace run-python subroutine with single find_python call #18621

wants to merge 1 commit into from

Conversation

seishun
Copy link
Contributor

@seishun seishun commented Feb 7, 2018

A subroutine does not work as a replacement for the python command
since one cannot use a subroutine call in a for /F loop.

This is an alternative to #17293 and #17804. I realized that find_python.cmd adds Python to PATH, so there is no need to introduce a variable.

I still slightly prefer #17293 for the reasons outlined in #17804 (basically, extra maintenance), but I prefer this PR to #17804 since it doesn't introduce an extra variable.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build

A subroutine does not work as a replacement for the `python` command
since one cannot use a subroutine call in a `for /F` loop.
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Feb 7, 2018
Copy link
Contributor

@bzoz bzoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit, otherwise LGTM!

@@ -165,6 +165,9 @@ if "%target%"=="Clean" echo deleting %~dp0deps\icu
if "%target%"=="Clean" rmdir /S /Q %~dp0deps\icu
:no-depsicu

call tools\msvs\find_python.cmd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe redirect to NUL, to avoid ERROR: The system was unable to find the specified registry key or value.?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like combining unrelated fixes in a single PR (this line was just moved from :run-python). Plus, I think it would be better to fix this inside find_python.cmd itself.

@bzoz
Copy link
Contributor

bzoz commented Feb 14, 2018

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM

@bzoz
Copy link
Contributor

bzoz commented Feb 15, 2018

Failures are unrelated.

@bzoz
Copy link
Contributor

bzoz commented Feb 15, 2018

...but the commit title is to too long. @seishun, could you make the commit title <50 chars long?

@seishun
Copy link
Contributor Author

seishun commented Feb 15, 2018

I don't think so, and it's not a requirement: https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines

preferably 50 characters or less, and no more than 72 characters

@bzoz
Copy link
Contributor

bzoz commented Feb 15, 2018

Landed in cfad441

@bzoz bzoz closed this Feb 15, 2018
bzoz pushed a commit that referenced this pull request Feb 15, 2018
A subroutine does not work as a replacement for the `python` command
since one cannot use a subroutine call in a `for /F` loop.

PR-URL: #18621
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@seishun seishun deleted the find_python branch February 16, 2018 19:01
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
A subroutine does not work as a replacement for the `python` command
since one cannot use a subroutine call in a `for /F` loop.

PR-URL: #18621
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
A subroutine does not work as a replacement for the `python` command
since one cannot use a subroutine call in a `for /F` loop.

PR-URL: #18621
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
A subroutine does not work as a replacement for the `python` command
since one cannot use a subroutine call in a `for /F` loop.

PR-URL: nodejs#18621
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@MylesBorins
Copy link
Contributor

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@seishun
Copy link
Contributor Author

seishun commented Aug 7, 2018

@MylesBorins seems pointless.

nodejs-github-bot pushed a commit that referenced this pull request Jan 6, 2022
PR-URL: #41235
Refs: #18621
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
targos pushed a commit that referenced this pull request Jan 14, 2022
PR-URL: #41235
Refs: #18621
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
PR-URL: nodejs#41235
Refs: nodejs#18621
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
PR-URL: #41235
Refs: #18621
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants