Skip to content

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Oct 29, 2024

This reverts commit 483cdcd.

This also adds some testing of the --separate-metadata file_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

@sbc100 sbc100 requested a review from kripken October 29, 2024 23:46
Copy link
Member

@kripken kripken left a 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?

@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 30, 2024

I've added a test as a separate commit. It also fixes --separate-metadata so that it runs under node. OK to land these together or should I land the test after the revert?

}
''')
self.do_runf('src.c', emcc_args=['--pre-js=immutable.js', '-sFORCE_FILESYSTEM'])

Copy link
Member

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.

Copy link
Member

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.

Copy link
Collaborator Author

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.

@sbc100 sbc100 changed the title Revert "Micro-optimize preRun/postRun handling. NFC (#22671)" file_packager: Fix regression in --separate-metadata output Oct 30, 2024
@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 30, 2024

Update the title to better reflect what this PR does

@kripken
Copy link
Member

kripken commented Oct 30, 2024

Thanks, lgtm

@sbc100 sbc100 merged commit c0575ef into emscripten-core:main Oct 30, 2024
28 checks passed
@sbc100 sbc100 deleted the revert branch October 30, 2024 19:40
sbc100 added a commit to sbc100/emscripten that referenced this pull request Aug 7, 2025
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
sbc100 added a commit to sbc100/emscripten that referenced this pull request Aug 8, 2025
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
sbc100 added a commit that referenced this pull request Aug 8, 2025
This was added back in #22802 but seems redundant since the check just
below for the indexedDB global should be enough to handle the node case.

Split out from #24883
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.

FS no longer populated after "Micro-optimize preRun/postRun handling. NFC (#22671)"

2 participants