gh-126691: Remove --with-emscripten-target #126787
Conversation
This unifies the code for nodejs and the code for the browser. After this commit, the browser example doesn't work. However, I have a fix for it prepared as a followup.
hoodmane
requested review from
brettcannon,
freakboy3742,
erlend-aasland and
corona10
as code owners
brettcannon
requested review from
mdboom
and removed request for
brettcannon
Misc/NEWS.d/next/Build/2024-11-13-15-47-09.gh-issue-126691.ni4K-b.rst
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I think this all makes sense; a couple of minor suggestions and requests for clarification inline. Those clarifications are mostly for my own edification - I'm still getting familiar with the internals of emscripten as a platform, so I want to make sure that I understand what is going on.
| Module.preRun = () => { | ||
| FS.mkdirTree("/lib/"); | ||
| FS.mount(NODEFS, { root: __dirname + "/lib/" }, "/lib/"); | ||
| }; |
There was a problem hiding this comment.
Is there a reason this wasn't needed previously?
There was a problem hiding this comment.
With -sNODERAWFS, the emulated file system is set up to match the native file system as closely as possible. So this is automatic. Now that we are not doing that, we need to mount the standard library so that the Python interpreter will start up. Ideally we'd be able to select whether or not to use -sNODERAWFS at runtime, but it's a node only option.
I think in a followup I'll update it to mount most of the native file system directories. But I want to change the way a few more things work first (and there are some bugs that make this a bit weirder than it could be emscripten-core/emscripten#22924).
| ] | ||
| ) | ||
| AS_VAR_APPEND([LDFLAGS_NODIST], [" --pre-js=\$(srcdir)/Tools/wasm/emscripten/node_pre.js"]) | ||
| WASM_LINKFORSHARED_DEBUG="-gseparate-dwarf --emit-symbol-map" |
There was a problem hiding this comment.
For my own edification - the summary of this set of changes is that we can essentially use the node compilation options everywhere:
- The
--preload-fileis not longer required; - The
os.pyreplacement in the standard library is no longer required; - We don't need the
-sALLOW_MEMORY_GROWTH,-sNODERAWFSand-sEXIT_RUNTIMEflags any more -gseparate-dwarfnow presumably works in Chrome Devtools.
There was a problem hiding this comment.
Yeah:
--preload-file
It is easy to use but it produces files in an ad-hoc file format that Emscripten made. We prefer to use tar or zip files, though it requires a tiny bit more setup. In Node, the plan is to use the NODEFS to mount the standard lib, see above. In the browser, we'll zip the standard library into python314.zip and put it in the file system at /lib/python314.zip then allow it to be imported via the zip importer.
os.py
Not really sure what this was for to be honest. Removing it doesn't cause any test failures as far as I can tell?
-sALLOW_MEMORY_GROWTH
It was a mistake to remove this, I think it will cause additional test failures.
-sNODERAWFS
It's incompatible with browsers so we can't use it if we want to share one build for both.
-sEXIT_RUNTIME
Well. I'm not actually sure on this one to be honest:
If 0, the runtime is not quit when
main()completes (allowing code to run
afterwards, for example from the browser main event loop).atexit()s are also
not executed, and we can avoid including code for runtime shutdown, like
flushing the stdio streams. Set this to 1 if you do wantatexit()s or stdio
streams to be flushed on exit.
It seems potentially helpful. I'm not sure what happens if you use exit_with_live_runtime() with -sEXIT_RUNTIME. My main reason for removing was that Pyodide doesn't pass it. But I think it'd be better to put it back for now and see what it does.
-gseparate-dwarf
Yes, it works in chrome devtools now. I'm a bit unclear also on why it was used in node before but not in chrome -- when I debug node, I go to chrome://inspect and click "Open dedicated DevTools for Node" so... yeah I thought the node debugger is chrome devtools too...
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
hoodmane
force-pushed
the
remove-emscripten-target
branch
from
12617ad
to
4516f3e
Compare
…K-b.rst Co-authored-by: Michael Droettboom <[email protected]>
|
@freakboy3742 can you review again? |
This unifies the code for nodejs and the code for the browser. After this commit, the browser example doesn't work because I deleted the code to copy the files around. However, I have a fix for this prepared as a followup.
--with-emscripten-target#126691📚 Documentation preview 📚: https://cpython-previews--126787.org.readthedocs.build/