-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: windows_boxstarter: "choco install python -y" for Python 3 #26424
Conversation
I would hold with that until all other Python 3 tasks are completed. |
Why do people want to wait? Everyone seems to have their favorite fix that they want us to hold off doing until tomorrow. 303 days until Python 2 end of life. |
Is there a reason for installing two different versions of Python? |
8a20dc4
to
e941e55
Compare
Yes. It is commonly done around the planet. In this instance, it gives our users the ability to test out node running on both Python and legacy Python. It is also done at nodejs/build#1644 on the node build machines and on all Travis CI xenial images. MacOS: brew install python python@2 Most Linux distros already include both Python and legacy Python (if they still ship with Python 2 at all) but just in case, commands like sudo apt install python3.7 python2.7 will work. |
IMHO this boxstarter script is for people who just want to be able to install native modules (as it is tied to a checkbox in installer), the "general public", not the developers. I think it should be kept minimal. Unless both Python versions are needed to build Node or native addons, we should stick to installing one version. |
That's not quite correct. This BoxStarter script is part of https://github.com/nodejs/node/tree/e941e556244ccea7bf46a76d1d18f865b73d3ea8/tools/bootstrap (linked to from https://github.com/nodejs/node/blob/e941e556244ccea7bf46a76d1d18f865b73d3ea8/BUILDING.md#building-nodejs-on-supported-platforms) and is for setting developers up for building Node.js (e.g. it installs nasm which is used for building openssl). The checkbox in the installer for the "general public" doesn't use BoxStarter anymore and calls Chocolatey directly: https://github.com/nodejs/node/blob/e941e556244ccea7bf46a76d1d18f865b73d3ea8/tools/msvs/install_tools/install_tools.bat. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case LGTM.
@nodejs/python @nodejs/platform-windows @nodejs/build-files @nodejs/tooling PTAL. This could use another review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, could you also update https://github.com/nodejs/node/blob/master/tools/bootstrap/README.md ? With note about reasons for having twon Python versions.
Python 2 will reach its _end-of-life_ at the end of 2019. Afterwards, the | ||
interpreter will not get updates -- no bugfixes, no security fixes, nothing. In | ||
the interim, the Python ecosystem is abandoning 2.7 support. | ||
https://python3statement.org/ In order to remain safe and current the Node.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this URL be hidden in a link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope.
PR-URL: nodejs#26424 Refs: nodejs#25789 (comment) Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: nodejs#26424 Refs: nodejs#25789 (comment) Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: #26424 Refs: #25789 (comment) Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
As suggested at #25789 (comment)
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes