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.

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: make nodedownload.py Python 3 compatible #29104

Closed
wants to merge 1 commit into from

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Aug 13, 2019

These changes make nodedownload.py compatible with both Python 2 and Python 3.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Aug 13, 2019
@cclauss cclauss added the python PRs and issues that require attention from people who are familiar with Python. label Aug 13, 2019
tools/configure.d/nodedownload.py Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@sam-github
Copy link
Contributor

@cclauss I force-pushed your branch after I squashed the commits and rewrote the commit message to start with tools: .

Also, kicked off a build.

I support fast-tracking this if it is green in CI.

@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member

Also, kicked off a build.

I support fast-tracking this if it is green in CI.

No objections to fast tracking but as far as I can remember this file is only used to download ICU which the normal builds don't do (they use the ICU in deps).

@cclauss
Copy link
Contributor Author

cclauss commented Aug 13, 2019

./confugure.py imports this file and seems to use its functions in multiple places.

https://github.com/nodejs/node/search?q=nodedownload&unscoped_q=nodedownload

@sam-github
Copy link
Contributor

@nodejs/platform-windows Above CI failure seems irrelevant to this PR, which only changed some python syntax: https://ci.nodejs.org/job/node-test-binary-windows-2/2533/ It looks to be related to temp file cleanup, which I think has an open windows PR somewhere to fix it, but I haven't been able to find it.

@richardlau
Copy link
Member

richardlau commented Aug 13, 2019

./confugure.py imports this file and seems to use its functions in multiple places.

https://github.com/nodejs/node/search?q=nodedownload&unscoped_q=nodedownload

From memory, only on paths where ICU is downloaded (e.g. if deps/icu-small isn't there (it is in the default git clone/source tree) or --download is used with configure).

@sam-github
Copy link
Contributor

If its tested and passed, seems OK to fast-track.

If its not covered by the tests... then leaving it not-fasttracked won't make it better :-)

Will a ./configure --download=all be sufficient for a manual test, @richardlau ? I've never used icu, much less built variants of it.

@richardlau
Copy link
Member

If its tested and passed, seems OK to fast-track.

If its not covered by the tests... then leaving it not-fasttracked won't make it better :-)

😁

Will a ./configure --download=all be sufficient for a manual test, @richardlau ? I've never used icu, much less built variants of it.

./configure --download=all --with-intl=full-icu should work (omitting --with-intl=... defaults to small-icu which uses deps/icu-small if it exists).

With this PR and python2:

bash-4.2$ ./configure --download=all --with-intl=full-icu
 <https://github.com/unicode-org/icu/releases/download/release-64-2/icu4c-64_2-src.tgz>
 Fetch: . 23.5MB total, 23.5MB downloaded
Checking file integrity with md5:
md5:      a3d18213beec454e3cdec9a3116d6b05  deps/icu4c-64_2-src.tgz
 Extracting tarfile: deps/icu4c-64_2-src.tgz
INFO: Using floating patch "tools/icu/patches/64/source/common/putil.cpp" from "tools/icu"
INFO: Using floating patch "tools/icu/patches/64/source/i18n/dtptngen.cpp" from "tools/icu"
INFO: configure completed successfully
bash-4.2$

So we should be no worse off than we are currently.

@sam-github
Copy link
Contributor

OK, well fast-track or not, its all green in CI, 5 approvals, can be landed in 44 hours.

@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. labels Aug 13, 2019
Copy link
Member

@MattIPv4 MattIPv4 left a comment

Choose a reason for hiding this comment

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

lgtm.

@Trott
Copy link
Member

Trott commented Aug 14, 2019

It looks to be related to temp file cleanup, which I think has an open windows PR somewhere to fix it

It landed. It's at #28858.

Windows tmpdir cleanup issues have been worked on by @joaocgreis and, before that, @refack.

@sam-github
Copy link
Contributor

I rebased this onto master, and kicked off another ci

@nodejs-github-bot
Copy link
Collaborator

@joaocgreis
Copy link
Member

None of the failures I see there seem to be related to this PR, and the following CI was green so this can land.

The failure in Windows 2008 shows a ENOTEMPTY for a directory that seems empty. I want to investigate things like this, so thanks for mentioning me.

The failures on other Windows versions show a problem with git fetching from the temp repo. Looking at Jenkins timestamps, the git push in the compile job completed before the test jobs started the git fetch, so this might be an issue in Jenkins or Git. We'll have to investigate this if it happens again.

@sam-github
Copy link
Contributor

Thanks for the analysis @joaocgreis -- and for the work, @cclauss

Landed in 8ae79c9

@sam-github sam-github closed this Aug 14, 2019
sam-github pushed a commit that referenced this pull request Aug 14, 2019
PR-URL: #29104
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@cclauss cclauss deleted the python3-push-again branch August 14, 2019 23:53
targos pushed a commit that referenced this pull request Aug 19, 2019
PR-URL: #29104
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BaochengSu pushed a commit to BaochengSu/node that referenced this pull request Oct 20, 2020
PR-URL: nodejs#29104
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
(cherry picked from commit 8ae79c9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. fast-track PRs that do not need to wait for 48 hours to land. python PRs and issues that require attention from people who are familiar with Python.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants