-
Notifications
You must be signed in to change notification settings - Fork 649
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
Update node version to 14.15.5 (latest TLS release) #708
Conversation
b095082
to
07419a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm aside from comments
emsdk.py
Outdated
print(os.path.exists(self.installation_path())) | ||
for p in activated_path: | ||
print(p) | ||
print(os.path.exists(p)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like debug code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup.. will remove before landing.
@@ -40,6 +40,7 @@ | |||
shutil.move(dirname, 'bin') | |||
os.mkdir(dirname) | |||
shutil.move('bin', dirname) | |||
os.remove(filename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't this break the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixed a bug where we were including two copies of node in windows. We were calling zip with the existing zip file present which was adding the new "bin" directory, but not removing the old zip contents. So it saves a bit of space for all windows users.
611c2ce
to
496ce10
Compare
emsdk.py
Outdated
errlog(os.path.exists(self.installation_path())) | ||
for p in activated_path: | ||
errlog(p) | ||
errlog(os.path.exists(p)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like errlog
prints to stderr - that still seems excessive here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm just debugging the windows failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm (with logging removed and assuming the windows fix does not change anything)
This test starts to fail with the upgrade: test_node_code_caching @kripken can you verify that this test fails with recent versions of node ? Is this a known issue? |
@sbc100 Oh, yes - this is a known issue, node changed/removed the API we depended on, nodejs/node#18265 (comment) . There is a note on this in settings.js. We can just disable the test, I guess. I hope node adds a new API at some point. |
Ok I will land this and roll a PR to disable the test. |
See WebAssembly/binaryen#3551