Skip to content

Cleanup file_packager python code #10177

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

Merged
merged 4 commits into from
Jan 9, 2020
Merged

Cleanup file_packager python code #10177

merged 4 commits into from
Jan 9, 2020

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jan 9, 2020

Also clean up the test code (test change should be NFC).

Also clean up the test code (test change should be NFC).
@sbc100 sbc100 requested a review from kripken January 9, 2020 18:45
# verify js output file is immutable when metadata is separated
shutil.copy2('immutable.js', 'immutable.js.copy') # copy with timestamp preserved
time.sleep(3.0)
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment justifying this, as in general a sleep like that looks very dangerous (but here it is indeed necessary, I agree).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops I certainly didn't mean to land that ..

Copy link
Member

Choose a reason for hiding this comment

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

No wait, I think it's good? It ensures the timestamp would be different, so it forces the test to actually check, instead of by virtue of being fast enough having the same time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it worth ? Isn't the really important thing that the contents don't change?

Anyway I put it back with a comment.

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 9, 2020

I think the test failure here is unrelated. Looks related to #10145.

OK to land (since I think this is causing failures on the emscripten-releases CI).

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 9, 2020

?

@kripken
Copy link
Member

kripken commented Jan 9, 2020

The error does look unrelated to this PR, yes, lgtm.

@sbc100 sbc100 merged commit 5b2fc36 into incoming Jan 9, 2020
@delete-merged-branch delete-merged-branch bot deleted the file_packager_fixes branch January 9, 2020 23:24
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
Also clean up the test code (test change should be NFC).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants