Skip to content

build,tools: Python3 compatibility and refactoring#26725

Closed
refack wants to merge 2 commits intonodejs:masterfrom
refack:refacotr-configure-for-python3
Closed

build,tools: Python3 compatibility and refactoring#26725
refack wants to merge 2 commits intonodejs:masterfrom
refack:refacotr-configure-for-python3

Conversation

@refack
Copy link
Contributor

@refack refack commented Mar 18, 2019

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 18, 2019

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2952/pipeline

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Mar 18, 2019
@refack refack added the python PRs and issues that require attention from people who are familiar with Python. label Mar 18, 2019
@refack refack added this to the 12.0.0 milestone Mar 18, 2019
@refack
Copy link
Contributor Author

refack commented Mar 18, 2019 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/21616/
CI: https://ci.nodejs.org/job/node-test-pull-request/21617/
Just Windows: https://ci.nodejs.org/job/node-test-commit-windows-fanned/25610/

/CC @nodejs/python @nodejs/build-files

Still has a few bugs. But still PTAL.

@refack refack added the wip Issues and PRs that are still a work in progress. label Mar 18, 2019
@refack refack force-pushed the refacotr-configure-for-python3 branch 2 times, most recently from b7a3e11 to 36f93f6 Compare March 18, 2019 01:47
@refack refack self-assigned this Mar 19, 2019
This was referenced Mar 19, 2019
Closed
Closed
@cclauss cclauss mentioned this pull request Mar 19, 2019
Closed
63 tasks
@refack refack force-pushed the refacotr-configure-for-python3 branch 4 times, most recently from 1fcf092 to aa3826a Compare March 19, 2019 23:49
@targos
Copy link
Member

targos commented Mar 21, 2019

Ping @nodejs/python

targos
targos reviewed Mar 21, 2019
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

Is the new and empty tools/configure_lib/__init__.py necessary?

tools/configure_lib/nodedownload.py Outdated
Copy link
Member

@targos targos Mar 21, 2019

Choose a reason for hiding this comment

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

URLopener seems unused

This was referenced Mar 22, 2019
Merged
Closed
@refack
Copy link
Contributor Author

refack commented Mar 22, 2019 •
edited
Loading

Is the new and empty tools/configure_lib/__init__.py necessary?

It's a python thing (it tells the runtime that that directory is a "package") that enables doing:
https://github.com/nodejs/node/blob/aa3826ae54ee2422ffc38ca1b94e8dea7f09de8c/configure.py#L25
instead of:

node/configure.py

Lines 30 to 32 in 2fa8dc4

# imports in tools/configure.d
sys.path.insert(0, os.path.join('tools', 'configure.d'))
import nodedownload

(which is not cricket)

thefourtheye
thefourtheye reviewed Mar 25, 2019
View reviewed changes
configure.py Outdated
Copy link
Contributor

@thefourtheye thefourtheye Mar 25, 2019

Choose a reason for hiding this comment

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

This is not necessary for Python 3 compatibility I believe. Can you explain why this is introduced?

configure.py Outdated
Copy link
Contributor

@thefourtheye thefourtheye Mar 25, 2019

Choose a reason for hiding this comment

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

Nit: There are a few lines which cross the 80 characters limit.

@thefourtheye
Copy link
Contributor

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.

@Trott Trott removed this from the 12.0.0 milestone Apr 29, 2019
This was referenced Apr 30, 2019
Merged
Closed
@refack refack removed the wip Issues and PRs that are still a work in progress. label May 6, 2019
@refack refack force-pushed the refacotr-configure-for-python3 branch from aa3826a to 9a8292e Compare May 6, 2019 11:59
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 6, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/22982/

targos
targos approved these changes May 24, 2019
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

LGTM with nits fixed and green CI

@targos
Copy link
Member

targos commented May 24, 2019

/cc @cclauss

@cclauss cclauss mentioned this pull request Oct 2, 2019
Closed
@vccolombo vccolombo mentioned this pull request Oct 2, 2019
Closed
4 tasks
@cclauss
Copy link
Contributor

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?

@Trott Trott closed this Nov 3, 2019
@RaisinTen RaisinTen mentioned this pull request Oct 22, 2020
Merged
3 tasks
Trott pushed a commit to RaisinTen/node that referenced this pull request Nov 12, 2020
@RaisinTen @Trott
build: refactor configure.py to use argparse
f5a86b5 
Refs: nodejs#26725
Fixes: nodejs#29813
Refs: nodejs#29814

PR-URL: nodejs#35755
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
codebytere pushed a commit that referenced this pull request Nov 22, 2020
@RaisinTen @codebytere
build: refactor configure.py to use argparse
56f83e6 
Refs: #26725
Fixes: #29813
Refs: #29814

PR-URL: #35755
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@targos targos targos approved these changes

@cclauss cclauss cclauss approved these changes

+1 more reviewer

@thefourtheye thefourtheye thefourtheye left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@refack refack

Labels

build Issues and PRs related to build files or the CI. python PRs and issues that require attention from people who are familiar with Python.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@refack @nodejs-github-bot @targos @thefourtheye @cclauss @Trott