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

Accept Python 3 on Windows #29236

Closed

Conversation

@joaocgreis
Copy link
Member

commented Aug 20, 2019

This loosely depends on #25878. Doesn't make much sense without it, but doesn't break either.

This allows Python 3 to be used, but only if it is the only Python version installed. #25878 does this for configure, this PR does for vcbuild.

If Python 3 is being used and configure fails, a warning is displayed.

cc @nodejs/platform-windows @nodejs/python @cclauss

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
joaocgreis added 2 commits Aug 20, 2019
build,win: find Python in paths with spaces
When looking for Python in the registry, as specified in PEP514,
this was not able to handle installations in a path with spaces,
like Program Files. This ensures the whole path is used, fixing the
issue.
build,win: accept Python 3 if 2 is not available
If there is no Python 2 available, use Python 3. This allows to test
running configure with Python 3.
@nodejs-github-bot

This comment was marked as outdated.

@joaocgreis joaocgreis referenced this pull request Aug 20, 2019
1 of 4 tasks complete
@nodejs-github-bot

This comment has been minimized.

Copy link

commented Aug 20, 2019

@cclauss

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

I am not a Windows guy so patience please...
If I go to the Microsoft Windows Store and install Python from there, where will it go?
My sense is that py is always set to the current preferred Python 3 on Windows boxes. Is that a better way to find py3?

@joaocgreis

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2019

If I go to the Microsoft Windows Store and install Python from there, where will it go?

Under %LocalAppData%\Microsoft\WindowsApps\, which is already in PATH.

My sense is that py is always set to the current preferred Python 3 on Windows boxes. Is that a better way to find py3?

Possibly, but isn't that mostly for people running multiple versions? Is that worth the complexity for our case? For node-gyp that would make more sense I think, if someone has the time to implement it.

Here, we should make sure an installation of Python with all things default works - that's why the PEP514 detection was added: nodejs/CTC#147 (comment). For someone wanting to customize, it's reasonable to assume they can add Python to the path.

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

@nodejs/python @nodejs/platform-windows @nodejs/build-files This could use some reviews.

It's not clear to me if this should be blocked on #25878 or not. Based on @joaocgreis's comment, I'd say not, but I'd love someone else's opinion (especially @joaocgreis's!).

@cclauss

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

What happens on a Windows box when this is run? How far does it get and what are the results? What cannot be achieved by doing python3 configure.py? Our current Python 3.6 runs on Travis Linux run for 50 minutes before timing out. I would be interested to see our builds get to some interesting place before we open the experimentation to a broad audience.

As discussed at #29196 (comment) there are several vectors of testing that I believe we should shape up before opening the floodgates with #25878 and #29236.

  1. PYTHON=python3 ; tools/gyp_node.py
  2. PYTHON=python3 ; tools/test.py
  3. python3 configure.py ; make test # current Travis CI run
  4. python3 configure.py ; make install
@PsychoXIVI

This comment was marked as disruptive content.

Copy link

commented Aug 25, 2019

%LocalAppData%\Microsoft\WindowsApps\ folder has new kind of shortcut/link files, including python.exe and python3.exe. These might be used to advertise their Store (as they provide Python for Windows from there). But, this is Microsoft, and it is so retarded, so even their own tools does not work with these. Powershell? Pff, "UNKNOWN", link file without Target. attrib? Nope, The target of the symbolic link C:\Users\PsychoX\AppData\Local\Microsoft\WindowsApps\python.exe does not exist. Windows is just piece of "not nice" software. Not worth to use for such things.

@cclauss

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2019

@cclauss cclauss referenced this pull request Aug 26, 2019
29 of 41 tasks complete
@cclauss
Copy link
Contributor

left a comment

Given the state of #25789, I think we are ready to land this one.

What are the results of python3.6 configure.py ?

@joaocgreis

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

I think this can land. Building with Python 3 does not work, but the error is clear and this allows for experimentation.

@cclauss running configure.py directly on Windows is not straightforward because the environment needs to be set up by vcbuild.bat. FWIW this is what happens:

C:\Users\Administrator\Desktop\node>C:\Users\Administrator\AppData\Local\Programs\Python\Python36\python.exe configure.py
�[1m�[31mERROR�[0m: Did not find a new enough assembler, install one or build with
       --openssl-no-asm.
       Please refer to BUILDING.md

Running this together with #25878 is a better option, this is what happens:

C:\Users\Administrator\Desktop\node>git checkout nodejs/node/master
...
C:\Users\Administrator\Desktop\node>git clean -fdx
...
C:\Users\Administrator\Desktop\node>git cherry-pick nodejs/node/pull/25878~4..nodejs/node/pull/25878
...
C:\Users\Administrator\Desktop\node>git cherry-pick nodejs/node/pull/29236~2..nodejs/node/pull/29236
...
C:\Users\Administrator\Desktop\node>vcbuild.bat
Looking for Python
Python found in C:\Users\Administrator\AppData\Local\Programs\Python\Python36\\python.exe
WARNING: Python 3 is not yet fully supported, to avoid issues Python 2 should be installed.
Looking for NASM
Looking for Visual Studio 2017
Found MSVS version 15.0
configure  --dest-cpu=x64
Node configure: Found Python 3.6.8...
�[1m�[32mINFO�[0m: Using floating patch "tools/icu/patches/64/source/common/putil.cpp" from "tools/icu"
�[1m�[32mINFO�[0m: Using floating patch "tools/icu/patches/64/source/i18n/dtptngen.cpp" from "tools/icu"
Traceback (most recent call last):
  File "configure", line 23, in <module>
    import configure
  File "C:\Users\Administrator\Desktop\node\configure.py", line 1716, in <module>
    run_gyp(gyp_args)
  File "tools\gyp_node.py", line 54, in run_gyp
    rc = gyp.main(args)
  File "tools\gyp\pylib\gyp\__init__.py", line 547, in main
    return gyp_main(args)
  File "tools\gyp\pylib\gyp\__init__.py", line 523, in gyp_main
    options.duplicate_basename_check)
  File "tools\gyp\pylib\gyp\__init__.py", line 100, in Load
    generator = __import__(generator_name, globals(), locals(), generator_name)
  File "tools\gyp\pylib\gyp\generator\msvs.py", line 17, in <module>
    import gyp.generator.ninja as ninja_generator
  File "tools\gyp\pylib\gyp\generator\ninja.py", line 23, in <module>
    from cStringIO import StringIO
ModuleNotFoundError: No module named 'cStringIO'
Failed to create vc project files.
Python 3 is not yet fully supported, to avoid issues Python 2 should be installed.

C:\Users\Administrator\Desktop\node>
@cclauss cclauss referenced this pull request Aug 29, 2019
1 of 4 tasks complete
@cclauss

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

Feel free to cherrypick in #29371 if it helps you make progress.

It would be cool if we could set up a Jenkins, Travis CI, or GitHub Actions-based CI, or other CI that exercised Python 3 on Windows.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

My review was requested but I don't think I'm the right person to sign off on this. My Windows shell scripting skills are close to non-existent.

@Trott Trott added the author ready label Aug 31, 2019

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

FWIW, since this has been open for more than a week, one approval is enough to land it as long as there isn't any opposition to it (which I'm not seeing any) and CI passes (which it did).

@@ -689,6 +689,9 @@ goto exit

:create-msvs-files-failed
echo Failed to create vc project files.
if %VCBUILD_PYTHON_VERSION%==3 (
echo Python 3 is not yet fully supported, to avoid issues Python 2 should be installed.

This comment has been minimized.

Copy link
@Trott

Trott Aug 31, 2019

Member

Nit that can totally be ignored, especially since this message eventually will go away anyway.

Suggested change
echo Python 3 is not yet fully supported, to avoid issues Python 2 should be installed.
echo Python 3 is not yet fully supported. To avoid issues, Python 2 should be installed.
@Trott
Trott approved these changes Aug 31, 2019
Copy link
Member

left a comment

A bit rubber-stamp-y of me since I don't have a Windows machine available to test on, but LGTM as far as I can tell.

Trott added a commit that referenced this pull request Sep 3, 2019
build,win: find Python in paths with spaces
When looking for Python in the registry, as specified in PEP514,
this was not able to handle installations in a path with spaces,
like Program Files. This ensures the whole path is used, fixing the
issue.

PR-URL: #29236
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Trott added a commit that referenced this pull request Sep 3, 2019
build,win: accept Python 3 if 2 is not available
If there is no Python 2 available, use Python 3. This allows to test
running configure with Python 3.

PR-URL: #29236
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Landed in ab841d5...d18b6a7

@Trott Trott closed this Sep 3, 2019

@joaocgreis

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

@Trott thanks for landing this!

BridgeAR added a commit that referenced this pull request Sep 3, 2019
build,win: find Python in paths with spaces
When looking for Python in the registry, as specified in PEP514,
this was not able to handle installations in a path with spaces,
like Program Files. This ensures the whole path is used, fixing the
issue.

PR-URL: #29236
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR added a commit that referenced this pull request Sep 3, 2019
build,win: accept Python 3 if 2 is not available
If there is no Python 2 available, use Python 3. This allows to test
running configure with Python 3.

PR-URL: #29236
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR referenced this pull request Sep 3, 2019
BridgeAR added a commit that referenced this pull request Sep 4, 2019
build,win: find Python in paths with spaces
When looking for Python in the registry, as specified in PEP514,
this was not able to handle installations in a path with spaces,
like Program Files. This ensures the whole path is used, fixing the
issue.

PR-URL: #29236
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR added a commit that referenced this pull request Sep 4, 2019
build,win: accept Python 3 if 2 is not available
If there is no Python 2 available, use Python 3. This allows to test
running configure with Python 3.

PR-URL: #29236
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
build,win: find Python in paths with spaces
When looking for Python in the registry, as specified in PEP514,
this was not able to handle installations in a path with spaces,
like Program Files. This ensures the whole path is used, fixing the
issue.

PR-URL: nodejs#29236
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
build,win: accept Python 3 if 2 is not available
If there is no Python 2 available, use Python 3. This allows to test
running configure with Python 3.

PR-URL: nodejs#29236
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
build,win: find Python in paths with spaces
When looking for Python in the registry, as specified in PEP514,
this was not able to handle installations in a path with spaces,
like Program Files. This ensures the whole path is used, fixing the
issue.

PR-URL: nodejs#29236
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
build,win: accept Python 3 if 2 is not available
If there is no Python 2 available, use Python 3. This allows to test
running configure with Python 3.

PR-URL: nodejs#29236
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.