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

build: make install.py python 3 compatiable #25583

Conversation

@thefourtheye
Copy link
Contributor

commented Jan 19, 2019

This patch replaces usage of filter in such a way that it will be
compatible with Python 3. Also, this patch replaces the usage of map
to do a side-effect work with normal for loop.

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

cc @nodejs/python @cclauss

build: make install.py python 3 compatiable
This patch replaces usage of `filter` in such a way that it will be
compatible with Python 3. Also, this patch replaces the usage of `map`
to do a side-effect work with normal `for` loop.
@refack
refack approved these changes Jan 19, 2019
Copy link
Member

left a comment

RSLGTM

@refack refack added the python label Jan 19, 2019

@@ -85,7 +90,7 @@ def npm_files(action):
# npm has a *lot* of files and it'd be a pain to maintain a fixed list here
# so we walk its source directory instead...
for dirname, subdirs, basenames in os.walk('deps/npm', topdown=True):
subdirs[:] = filter('test'.__ne__, subdirs) # skip test suites
subdirs[:] = [subdir for subdir in subdirs if subdir != 'test']

This comment has been minimized.

Copy link
@cclauss

cclauss Jan 19, 2019

Contributor

Is the [:] needed?

This comment has been minimized.

Copy link
@refack

refack Jan 19, 2019

Member

@thefourtheye could you put a TODO here. I actually think we should not mess with internal npm directories at all.

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Jan 20, 2019

Author Contributor

@cclauss I believe it is intentional. We are just ignoring the test directories and we do not want the os.walk to traverse them. If we drop [:], it will not change the actual list of directories to be traversed.

Quoting os.walk,

When topdown is True, the caller can modify the dirnames list in-place (perhaps using del or slice assignment), and walk() will only recurse into the subdirectories whose names remain in dirnames; this can be used to prune the search, impose a specific order of visiting, or even to inform walk() about directories the caller creates or renames before it resumes walk() again.

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Jan 20, 2019

Author Contributor

@refack The comment above the for loop explains why it is done. Isn't it a good reason to do this?

This comment has been minimized.

Copy link
@refack

refack Jan 20, 2019

Member

The comment above the for loop explains why it is done.

IIUC the comment explains why we walk, not why the output is filtered ¯\(ツ)/¯, IMHO npm should be a black-box and we should install it as is. But sorry for the off-topic comment.

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Jan 21, 2019

Member

I wrote that comment. Perhaps I should've added "...and filter what we don't need" but that was kind of self-evident when there was still a call to filter(). :)

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Jan 22, 2019

Author Contributor

@refack We wouldn't want to install the unit tests in npm, right?

This comment has been minimized.

Copy link
@refack

refack Jan 22, 2019

Member

Since ATM the process for vendoring npm is to extract the release tarball, personally I'd say we should install the whole tree as a black box (So it's equivalent to npm i -g npm).
But IMO we should follow up that discussion in a separate PR.

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Jan 22, 2019

Author Contributor

@refack Sure, yeah. We might want to hear from other collaborators as well.

@cclauss

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2019

@bricss Instead of a 👍 could you please consider doing a Review changes because that will give project maintainers more confidence that you has read through these code changes and believe that they improve the state of the project?

Anyone can review any GitHub pull request:

  1. Scroll up on this page and click on Files changed just above the initial commit message.
  2. Click on the green button Review changes
  3. Click on Comment or Approve or Request changes

Thanks for taking the time to go through these step which should help project maintainers.

@refack refack added the author ready label Jan 20, 2019

@refack

This comment has been minimized.

@danbev

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

Landed in 907ff0a.

@danbev danbev closed this Jan 23, 2019

danbev added a commit that referenced this pull request Jan 23, 2019
build: make install.py python 3 compatiable
This patch replaces usage of `filter` in such a way that it will be
compatible with Python 3. Also, this patch replaces the usage of `map`
to do a side-effect work with normal `for` loop.

PR-URL: #25583
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>

@thefourtheye thefourtheye deleted the thefourtheye:make-install.py-python3-compatible branch Jan 23, 2019

addaleax added a commit that referenced this pull request Jan 23, 2019
build: make install.py python 3 compatiable
This patch replaces usage of `filter` in such a way that it will be
compatible with Python 3. Also, this patch replaces the usage of `map`
to do a side-effect work with normal `for` loop.

PR-URL: #25583
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.