build: refactor configure.py #47323

VoltrexKeyva · 2023-03-30T17:14:56Z

Explicitly specify the encoding when opening files.
Use f-strings to format strings.
Use isinstance() for type checks instead of type().
Use the with keyword for resource-allocating operations.
Avoid using multiple statements in a single line.
Remove unnecessary else clauses after return.
Iterate with the items() method of dictionaries when both the key and value are used.
Remove unnecessary parentheses.
Rename unused unpacked variables to _, _1, etc etc.
Rename the list variable to avoid conflict with the global list() function.
Remove unused path parameter of the icu_download() function.
Use the pathlib library for paths instead of os.path.

VoltrexKeyva · 2023-03-30T17:25:25Z

Note that the tests are failing because the packaging library is not installed, I'm not sure where to add the command to install it so that it applies to all the tests that run configure.py.

richardlau · 2023-03-30T18:27:31Z

Replace usage of the deprecated distutils library with the packaging library recommended by setuptools' "Porting from Distutils" guide.

FWIW some previous discussion in #42186. packaging not being part of the Python stdlib is going to be a pain for everyone/new contributors.

VoltrexKeyva · 2023-03-31T18:09:35Z

packaging not being part of the Python stdlib is going to be a pain for everyone/new contributors.

I'm not sure about that, we'll have to switch either way though we could make our own class/function implementing version parsing but it's too much work for the simple use cases here, so I'd say we should make the move to the packaging library, and of course, add it to the build instructions if automatic installation is not a preferable choice.

configure.py

VoltrexKeyva · 2023-04-05T14:34:39Z

configure.py

-  def icu_download(path):
+  def icu_download():


I'm not sure about this change, the path parameter of the icu_download() function was never used (not inside the function I mean), not even from the day it was added.

But an argument to the path parameter is being passed here:

node/configure.py

Line 1819 in 069365c

localzip = icu_download(icu_full_path)

even though it's not used inside the icu_download() function, is this a mistake or was it supposed to be used?

👍 Let's revert this change and put a # noqa here to placate the linter.

Why should we revert this? Do you want me to make this change in a separate PR?

targos · 2023-04-12T08:19:27Z

@nodejs/python

configure.py

cclauss

This is a legacy codebase that a lot of folks depend on so we want to keep any possible breakage to a minimum. Here are some ideas...

Also, I would like to land #47519 before we land this so we have some stronger guardrails in place.

configure.py

cclauss · 2023-04-12T08:42:39Z

configure.py

-  def icu_download(path):
+  def icu_download():


👍 Let's revert this change and put a # noqa here to placate the linter.

configure.py

cclauss

more ideas

I know we are only changing a single file but it is a vital file and I do not know if it has enough tests... Perhaps we should introduce these changes in multiple PRs. Your thoughts on test coverage?

configure.py

- Explicitly specify the encoding when opening files. - Use f-strings to format strings. - Use `isinstance()` for type checks instead of `type()`. - Use the `with` keyword for resource-allocating operations. - Avoid using multiple statements in a single line. - Remove unnecessary `else` clauses after `return`. - Iterate with the `items()` method of dictionaries when both the key and value are used. - Remove unnecessary parentheses. - Rename unused unpacked variables to `_`, `_1`, etc etc. - Rename the `list` variable to avoid conflict with the global `list()` function. - Remove unused `path` parameter of the `icu_download()` function. - Use the `pathlib` library for paths instead of `os.path`.

VoltrexKeyva · 2023-04-16T11:49:43Z

Perhaps we should introduce these changes in multiple PRs.

I'm fine with that, should I introduce each of these changes in separate PRs to reduce review time?

Your thoughts on test coverage?

Writing new tests for such build files sounds like a good idea, but I have no experience writing tests for such files 😅.

cclauss · 2023-04-16T11:48:28Z

configure.py

@@ -53,8 +56,7 @@
 valid_mips_fpu = ('fp32', 'fp64', 'fpxx')
 valid_mips_float_abi = ('soft', 'hard')
 valid_intl_modes = ('none', 'small-icu', 'full-icu', 'system-icu')
-with open ('tools/icu/icu_versions.json') as f:
-  icu_versions = json.load(f)
+icu_versions = json.loads((tools_path / 'icu' / 'icu_versions.json').read_text())


.read_text(encoding='utf-8') is also possible.

cclauss · 2023-04-16T11:57:02Z

I think we should get this PR will all changes ready and with all tests passing. Then we should have a second PR that is a strict subset of these changes. We land the second PR first and then after a release and user feedback, we rebase this PR and push the remaining into the next Node release. All the pathlib changes should land separately from all other changes. Thoughts? Or am I just being too paranoid?

VoltrexKeyva · 2023-04-16T11:59:42Z

Then we should have a second PR that is a strict subset of these changes.

What changes here counts as a strict subset?

All the pathlib changes should land separately from all other changes. Thoughts? Or am I just being too paranoid?

That's a good idea.

cclauss · 2023-04-16T12:18:21Z

What changes here counts as a strict subset?

The pathlib changes vs. all non-pathlib changes.

Use Python's `pathlib` library for paths and related operations instead of `os.path`. Refs: nodejs#47323 (comment) nodejs#47323 (comment)

VoltrexKeyva · 2023-04-22T08:50:20Z

Closing this since there are now two different PRs, one for pathlib changes (#47581) and one for non-pathlib changes (#47667).

Use Python's `pathlib` library for paths and related operations instead of `os.path`. Refs: nodejs#47323 (comment) nodejs#47323 (comment)

Use Python's `pathlib` library for paths and related operations instead of `os.path`. Refs: #47323 (comment) #47323 (comment) PR-URL: #47581 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Christian Clauss <cclauss@me.com>

Use Python's `pathlib` library for paths and related operations instead of `os.path`. Refs: nodejs#47323 (comment) nodejs#47323 (comment) PR-URL: nodejs#47581 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Christian Clauss <cclauss@me.com>

nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels Mar 30, 2023

VoltrexKeyva force-pushed the refact-conf branch 2 times, most recently from 762c432 to 14b08f7 Compare March 30, 2023 17:32

Trott added the python PRs and issues that require attention from people who are familiar with Python. label Mar 30, 2023

VoltrexKeyva force-pushed the refact-conf branch from 14b08f7 to 4863d58 Compare April 4, 2023 15:15

targos reviewed Apr 5, 2023

View reviewed changes

configure.py Outdated Show resolved Hide resolved

VoltrexKeyva force-pushed the refact-conf branch 3 times, most recently from f2d5605 to e6931de Compare April 5, 2023 14:23

VoltrexKeyva commented Apr 5, 2023

View reviewed changes

VoltrexKeyva force-pushed the refact-conf branch from e6931de to 9a0a399 Compare April 10, 2023 08:39

VoltrexKeyva requested review from targos and richardlau April 10, 2023 22:21

targos removed their request for review April 12, 2023 08:19

cclauss reviewed Apr 12, 2023

View reviewed changes

configure.py Outdated Show resolved Hide resolved

cclauss requested changes Apr 12, 2023

View reviewed changes

VoltrexKeyva force-pushed the refact-conf branch 2 times, most recently from ed5706f to 89e91ed Compare April 15, 2023 10:07

This comment was marked as outdated.

Sign in to view

VoltrexKeyva force-pushed the refact-conf branch 3 times, most recently from edf05e6 to ff0ff9b Compare April 16, 2023 10:08

cclauss requested changes Apr 16, 2023

View reviewed changes

configure.py Outdated Show resolved Hide resolved

configure.py Outdated Show resolved Hide resolved

configure.py Outdated Show resolved Hide resolved

configure.py Outdated Show resolved Hide resolved

configure.py Outdated Show resolved Hide resolved

configure.py Show resolved Hide resolved

VoltrexKeyva force-pushed the refact-conf branch 2 times, most recently from dec8698 to f3c8e7f Compare April 16, 2023 11:23

VoltrexKeyva force-pushed the refact-conf branch from f3c8e7f to 68ef467 Compare April 16, 2023 11:46

cclauss reviewed Apr 16, 2023

View reviewed changes

VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Apr 16, 2023

build: use pathlib for paths

a2d42dd

Use Python's `pathlib` library for paths and related operations instead of `os.path`. Refs: nodejs#47323 (comment) nodejs#47323 (comment)

VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Apr 16, 2023

build: use pathlib for paths

6d9d20c

Use Python's `pathlib` library for paths and related operations instead of `os.path`. Refs: nodejs#47323 (comment) nodejs#47323 (comment)

VoltrexKeyva mentioned this pull request Apr 16, 2023

build: use pathlib for paths #47581

Merged

VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Apr 17, 2023

build: use pathlib for paths

495b1fc

Use Python's `pathlib` library for paths and related operations instead of `os.path`. Refs: nodejs#47323 (comment) nodejs#47323 (comment)

VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Apr 17, 2023

build: use pathlib for paths

aa2fe83

Use Python's `pathlib` library for paths and related operations instead of `os.path`. Refs: nodejs#47323 (comment) nodejs#47323 (comment)

VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Apr 18, 2023

build: use pathlib for paths

fbc3909

Use Python's `pathlib` library for paths and related operations instead of `os.path`. Refs: nodejs#47323 (comment) nodejs#47323 (comment)

VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Apr 21, 2023

build: use pathlib for paths

289a07d

Use Python's `pathlib` library for paths and related operations instead of `os.path`. Refs: nodejs#47323 (comment) nodejs#47323 (comment)

VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Apr 21, 2023

build: use pathlib for paths

66e508a

Use Python's `pathlib` library for paths and related operations instead of `os.path`. Refs: nodejs#47323 (comment) nodejs#47323 (comment)

VoltrexKeyva closed this Apr 22, 2023

VoltrexKeyva deleted the refact-conf branch April 22, 2023 08:50

VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request May 1, 2023

build: use pathlib for paths

7856c1d

Use Python's `pathlib` library for paths and related operations instead of `os.path`. Refs: nodejs#47323 (comment) nodejs#47323 (comment)

VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request May 2, 2023

build: use pathlib for paths

ec01f70

Use Python's `pathlib` library for paths and related operations instead of `os.path`. Refs: nodejs#47323 (comment) nodejs#47323 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: refactor configure.py #47323

build: refactor configure.py #47323

VoltrexKeyva commented Mar 30, 2023 •

edited

VoltrexKeyva commented Mar 30, 2023

richardlau commented Mar 30, 2023

VoltrexKeyva commented Mar 31, 2023

VoltrexKeyva Apr 5, 2023 •

edited

cclauss Apr 12, 2023

VoltrexKeyva Apr 16, 2023

targos commented Apr 12, 2023

cclauss left a comment •

edited

cclauss Apr 12, 2023

This comment was marked as outdated.

cclauss left a comment

VoltrexKeyva commented Apr 16, 2023

cclauss Apr 16, 2023

cclauss commented Apr 16, 2023 •

edited

VoltrexKeyva commented Apr 16, 2023

cclauss commented Apr 16, 2023

VoltrexKeyva commented Apr 22, 2023

build: refactor configure.py #47323

build: refactor configure.py #47323

Conversation

VoltrexKeyva commented Mar 30, 2023 • edited

VoltrexKeyva commented Mar 30, 2023

richardlau commented Mar 30, 2023

VoltrexKeyva commented Mar 31, 2023

VoltrexKeyva Apr 5, 2023 • edited

Choose a reason for hiding this comment

cclauss Apr 12, 2023

Choose a reason for hiding this comment

VoltrexKeyva Apr 16, 2023

Choose a reason for hiding this comment

targos commented Apr 12, 2023

cclauss left a comment • edited

Choose a reason for hiding this comment

cclauss Apr 12, 2023

Choose a reason for hiding this comment

This comment was marked as outdated.

cclauss left a comment

Choose a reason for hiding this comment

VoltrexKeyva commented Apr 16, 2023

cclauss Apr 16, 2023

Choose a reason for hiding this comment

cclauss commented Apr 16, 2023 • edited

VoltrexKeyva commented Apr 16, 2023

cclauss commented Apr 16, 2023

VoltrexKeyva commented Apr 22, 2023

VoltrexKeyva commented Mar 30, 2023 •

edited

VoltrexKeyva Apr 5, 2023 •

edited

cclauss left a comment •

edited

cclauss commented Apr 16, 2023 •

edited