-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
tools,win: fix find_python error #22797
Conversation
cc @nodejs/platform-windows |
I did some local testing and found that b6e991d is required as well. |
Oh, interesting, what was the symptom there? I only have Python 2.7 installed on this machine (but not in the PATH), so it's possible that I didn't run into that issue. |
I had both 3.7 & 2.7 (and none in the Path), and I found that the path is not propagated
in the full
P.S. Let me test it a little bit more... |
I appreciate the help testing this out, I suspect this code doesn't get too much exercise. |
I think my fix is good either way, AFAICT the script is completely broken without the fix (if you don't have python.exe in your path). @refack, did you get a chance to determine whether your fix was necessary? Any issues you've come across. I'm inclined to land this today as long as the CI looks good and there are no objections. We can tweak this further if people hit other issues. |
Rebased and merged @refack's commits together. Will kick off a CI if Jenkins responds. |
@refack, trying it out again today, it seems like your change causes my machine to not find Python at all. |
Ok. |
@kfarnung does the |
I do use PowerShell most of the time, but both CMD and PowerShell exhibit the same behavior (that's fixed by my change). I have Python 2.7 installed in Here's the output of before (vcbuild_before.txt) and after (vcbuild_fixed.txt) my fix. |
Resumed Windows: https://ci.nodejs.org/job/node-test-commit-windows-fanned/20796/ |
That was wise, I see the edge case, you have 3.7 only in |
I suspected that might be the case. Given that I have a clean CI now, would you be ok with my landing this PR? |
Sure, if it fixes your system, it's definatly an improvement. |
Thanks! Just for reference, how do you have Python installed? Are they both under HKCU? |
Yes, but I hand roll my setup. I use P.S. |
Landed in 988be43 |
On a machine without `python.exe` in the PATH the script was failing with: ```console > .\vcbuild.bat Looking for Python 2.x 2> was unexpected at this time. ``` Escaping the `>` seems to resolve it. PR-URL: #22797 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
On a machine without `python.exe` in the PATH the script was failing with: ```console > .\vcbuild.bat Looking for Python 2.x 2> was unexpected at this time. ``` Escaping the `>` seems to resolve it. PR-URL: #22797 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
On a machine without `python.exe` in the PATH the script was failing with: ```console > .\vcbuild.bat Looking for Python 2.x 2> was unexpected at this time. ``` Escaping the `>` seems to resolve it. PR-URL: #22797 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
On a machine without
python.exe
in the PATH the script was failingwith:
Escaping the
>
seems to resolve it.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes