-
Notifications
You must be signed in to change notification settings - Fork 3.5k
file_packager: Fix regression in --separate-metadata output #22802
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
Conversation
kripken
left a comment
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.
Can we add a test?
|
I've added a test as a separate commit. It also fixes |
| } | ||
| ''') | ||
| self.do_runf('src.c', emcc_args=['--pre-js=immutable.js', '-sFORCE_FILESYSTEM']) | ||
|
|
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.
I'm not opposed to adding the test + node fix in this PR, that sgtm, but can you add an explanation for the test? It just appends to an existing test and I can't figure out what specifically it is testing, and why it failed before this reversion.
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.
I mean that it looks like many other tests we already have coverage for, so I'm missing what is special 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.
There were no other node tests that ran with --separate-metadata. It was that options specifically that was broken.
|
Update the title to better reflect what this PR does |
|
Thanks, lgtm |
This was added back in emscripten-core#22802 but seems redundant since the check just below for the indexedDB global should be enough to handle the node case. Split out from emscripten-core#24883
This was added back in emscripten-core#22802 but seems redundant since the check just below for the indexedDB global should be enough to handle the node case. Split out from emscripten-core#24883
This reverts commit 483cdcd.
This also adds some testing of the
--separate-metadatafile_packager options where we actually run the generated code. As part of adding this test I also made the code work under node.Fixes: #22790