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

build: make install.py python 3 compatiable #25583

Closed
wants to merge 1 commit into from
Closed

build: make install.py python 3 compatiable #25583

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions tools/install.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,13 @@ def try_remove(path, dst):
try_unlink(target_path)
try_rmdir_r(os.path.dirname(target_path))

def install(paths, dst): map(lambda path: try_copy(path, dst), paths)
def uninstall(paths, dst): map(lambda path: try_remove(path, dst), paths)
def install(paths, dst):
for path in paths:
try_copy(path, dst)

def uninstall(paths, dst):
for path in paths:
try_remove(path, dst)

def npm_files(action):
target_path = 'lib/node_modules/npm/'
Expand All @@ -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']
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the [:] needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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(). :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

paths = [os.path.join(dirname, basename) for basename in basenames]
action(paths, target_path + dirname[9:] + '/')

Expand Down Expand Up @@ -162,7 +167,7 @@ def ignore_inspector_headers(files, dest):
'deps/v8/include/v8-inspector.h',
'deps/v8/include/v8-inspector-protocol.h'
]
files = filter(lambda name: name not in inspector_headers, files)
files = [name for name in files if name not in inspector_headers]
action(files, dest)

action([
Expand Down