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.

Sign up for GitHub

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

Jump to bottom

build,tools: Python3 compatibility and refactoring #26725

Closed
wants to merge 2 commits into from
Closed

build,tools: Python3 compatibility and refactoring #26725

wants to merge 2 commits into from

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

@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
try:
from urllib.request import FancyURLopener
except ImportError:
from urllib import FancyURLopener, URLopener
Copy link
Member

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
action='store_true',
default=False,
help='compile V8 with minimal optimizations and with runtime checks')
parser.add_argument('extra_gyp_args',
Copy link
Contributor

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
parser = argparse.ArgumentParser()
shared_optgroup = parser.add_argument_group(
"Shared libraries",
"Flags that allows you to control whether you want to build against built-in dependencies"
Copy link
Contributor

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

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

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

@thefourtheye thefourtheye thefourtheye left review comments

@cclauss cclauss cclauss approved these changes

@targos targos targos approved these changes

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