build,tools: Python3 compatibility and refactoring#26725
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added the build 
refack added the python
/CC @nodejs/python @nodejs/build-files Still has a few bugs. But still PTAL. |
refackforce-pushed the refacotr-configure-for-python3 branch 2 times, most recently from b7a3e11 to 36f93f6Compare
refackforce-pushed the refacotr-configure-for-python3 branch 4 times, most recently from 1fcf092 to aa3826aCompare
targos commented Mar 21, 2019
Ping @nodejs/python |
targos left a comment
targos left a comment There was a problem hiding this comment.
Is the new and empty tools/configure_lib/__init__.py necessary?
tools/configure_lib/nodedownload.py Outdated
It's a python thing (it tells the runtime that that directory is a "package") that enables doing: Lines 30 to 32 in 2fa8dc4 (which is not cricket) |
configure.py Outdated
There was a problem hiding this comment.
This is not necessary for Python 3 compatibility I believe. Can you explain why this is introduced?
configure.py Outdated
There was a problem hiding this comment.
Nit: There are a few lines which cross the 80 characters limit.
thefourtheye commented Mar 25, 2019
I feel that the refactoring can be done in a separate PR. Not introducing any new additional code would be easy for us to debug problems. |
refackforce-pushed the refacotr-configure-for-python3 branch from aa3826a to 9a8292eCompare
nodejs-github-bot commented May 6, 2019
targos left a comment
targos left a comment There was a problem hiding this comment.
LGTM with nits fixed and green CI
targos commented May 24, 2019
/cc @cclauss |
cclauss commented Nov 1, 2019
My vote would be that we close this. We have touched these files in the interim and most of these changes have been done. Is there anything remaining that we should consider? |
Refs: nodejs#26725Fixes: nodejs#29813 Refs: nodejs#29814 PR-URL: nodejs#35755 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Christian Clauss <[email protected]>
Refs: #26725Fixes: #29813 Refs: #29814 PR-URL: #35755 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Christian Clauss <[email protected]>
6 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes