-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
(Emscripten) Modularize the JavaScript and clean up the web build #15688
(Emscripten) Modularize the JavaScript and clean up the web build #15688
Conversation
0e340c5
to
1b93c25
Compare
This is looking great! So happy to have more people involved on emscripten. Looks like the github actions build is failing....
|
Yes, this is because the emsdk version on the emscripten build container from gitlab is ancient. I’m using builds made from a modern emsdk and they seem fine. A bit more discussion is on the standalone build fix PR, but that’s included in this PR already: #15695 |
There's a bug with the EXPORT_ES6 part which is due to the use of strict mode, I'll investigate. |
Fixed the mentioned bug, it prevented (among other things) fast-forward from working. |
This one also builds in the new container 👍 thanks! |
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 is great, thanks so much. Happy to have more focus on the web build 👍
Looks great! This one subsumes #15695 so if this is merged we can close that one. |
@JoeOsborn Ran into some build errors on some cores after upgrading emscripten. Do you happen to know what's wrong with it? |
My guess is something to do with symbol mangling if some cores are in C++ and some are in C, but that’s a wild guess. I’ll test tomorrow with the libretro super repo and try to compile that core. Is it weird that beetle-vb works but beetle-lynx doesn’t? |
The issue was that the lynx core was using em++ to combine objects into an archive file where it should have used emar, the patch to libretro-mednafen_lynx is like so (same for libretro-mednafen_pcfx, libretro-mednafen_vb, mednafen_supergrafx, ngp, pce, wswan):
For libretro-atari800:
libretro-bsnes_performance:
libretro-81:
libretro-bsnes_accuracy:
libretro-bsnes_balanced:
libretro-bsnes_cplusplus98:
libretro-bsnes:
This is getting kind of repetitive, all the bsnes/mercury cores need the same Makefile changes. libretro-freechaf:
libretro-cannonball:
mame2000:
mrboom:
neocd:
reminiscence:
quasi88:
mame-2015:
smsplus:
tyrquake:
vice:
|
Phew. I think I've gone through all the cores that had special-case logic for emscripten and made sure they all compile under the latest SDK. The SDK has changed C++ standards/clang versions since then and has become more restrictive with respect to the use of em++/emcc vs emar for archive files. I didn't fork each repo and open a PR because ... it sounded like a lot of work. Sorry. But these patches should do it. |
Wow, that's incredible work. Thanks so much, JoeOsborn. I can help push up some of these PRs. Do you think these patches will work on the older emscripten prior to our upgrade? Then we could make sure things are patched beforehand.
|
Happy to do it—it scratches my own itch too. I think these patches should work on the older sdk too, since some cores did use emar properly while others used emcc/em++. What would be the most expedient way to get these all applied? Should we split them half and half, or can you push PR branches directly to the repos, or what? |
I am unable to merge patches, but can help on creating the Pull Requests. |
I'm going up from the bottom of the list; everything from freechaf through vice has open PRs now. |
That's the rest of them, once those PRs are merged I think every core that was compiling before under emscripten will compile under the new emscripten SDK. |
What is this waiting on for merging into master? Do the PRs on the core repositories need to be merged first? |
Will this cause issues for our buildbot? What's the current state of teh deployed version on web.libretro.com? |
I don’t know how or when web.libretro.com is packaged/updated, I couldn’t tell from the gitlab-ci scripts. If this change and my per-core fixes (for the handful of cores with incorrect usage of emcc/em++ instead of emar) go through then it should be fine. This PR updates pkg/emscripten/index.html as needed. |
…en builds broken since de45fc2
This way focus means focus and we can have multiple RA instances in one page.
c3cf594
to
428055c
Compare
…bretro#15688) * Increase emscripten stack size and decrease path size to fix emscripten builds broken since de45fc2 * use modularize flags for better-behaved javascript output * makefile and loader changes * use specialHTMLTargets to support modular access to canvas * bind key events to canvas, not document This way focus means focus and we can have multiple RA instances in one page. * Work around an emscripten bug in strict mode * (Emscripten) Use console.error() for error messages * increase asyncify stack size * Fix `-lm` flag-related compile warnings in emscripten --------- Co-authored-by: Rob Loach <[email protected]>
This adds the -s MODULARIZE flag and some related flags to emscripten, and updates libretro.js and canvas targeting in C code to account for the fact that there is no longer a global Module object.
I want this patch for two reasons: