-
-
Notifications
You must be signed in to change notification settings - Fork 30.6k
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
gh-126898: Emscripten support: Use es6 modules #126903
Conversation
We delete `node_pre.js` and `--pre-js`. Instead that logic moves to `node_entry.mjs`. In order to access `FS` from outside of `python.mjs`, we need to add `-sEXPORTED_RUNTIME_METHODS=FS`. Instead of invoking `python.js` directly, we will invoke `node_entry.mjs`. Special care has to be taken to ensure that we get `sys.executable` correct. It should point to the original path that was invoked, without symlinks resolved. I updated the generated `python.sh` to calculate it's own absolute path with `realpath -s $0` and pass that in. Then `node_entry.mjs` sets `thisProgram` to the path passed in. I manually validated this by creating a symlink to `python.sh` and invoking it. `sys.executable` and `sys._base_exectable` indeed point to the symlink, not to the build directory `python.sh` and not to `node_entry.mjs`.
3a57bec
to
48eacca
Compare
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.
On adminstrative note, this is enough of a change to warrant a change note.
The polyglot script looks like a neat trick, but I think I agree that having separate scripts is less confusing.
Other than that, I think this all makes sense; a couple of questions (as before, mostly to confirm my own understanding of things):
- Is the
.mpy
extension a sufficient accepted convention at this point? The MDN docs note some potential problems with using.mjs
; but it looks like that's mostly an issue with older web servers. I presume (based on you making this change) that you're not concerned about that? - Related to the naming - is the
node_entry.mjs
exclusive to Node? AIUI, this script exists if anyone wants to use node as the entry point, with CPython's own test suite being the major use case for this; but an in-browser implementation (such as pyodide) would need to provide it's own "entry" script (which is one of the thingspyodide.mjs
is doing). Have I understood that relationship correctly?
If I've understood those two points correctly, then I think the naming scheme you've suggested here makes sense. The only potential downside I can see is long term - that the name python.mjs
isn't (and can't be, going forward) an entry point. If Python as a project ever made "browser-hosted python" an official download, it would make sense for that name to be python.(m)js
. However, that's also a sufficiently "future" problem that can be addressed with a file rename, so I'm not sure that's enough to stand in the way of progress right now.
Yeah it's universally supported in my experience. Browsers don't care which you use. OTOH, node is fairly insistent that you must use it. There are possible workarounds if you want to use
Yes. It could potentially be used by other server-like JS runtimes (bun, deno, etc). But for browser and browser-like JS runtimes it won't work. My next PR after this one will make the browser example work again, but I'll be aiming at keeping the changes as small as possible to restore the original feature set of the browser example.
Agreed happy to rename if/when we feel like it. Eventually a significant chunk of the Pyodide runtime could get upstreamed into Python. Until that occurs, I think it wouldn't be great to distribute Emscripten Python directly because for most people Pyodide is much better and it's not great to have an official worse alternative. |
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.
Eventually a significant chunk of the Pyodide runtime could get upstreamed into Python. Until that occurs, I think it wouldn't be great to distribute Emscripten Python directly because for most people Pyodide is much better and it's not great to have an official worse alternative.
Agreed that upstreaming Pyodide's modifications should be the long term goal - and that until that happens, developing an official CPython download/distribution doesn't make much sense. On that basis, any naming issue is a long term issue.
On that basis, I think my concerns/questions have all been addressed; this looks good to me!
Thanks Russel! |
We delete
node_pre.js
and--pre-js
and move that logic tonode_entry.mjs
. In order to accessFS
from outside ofpython.mjs
, we need to add-sEXPORTED_RUNTIME_METHODS=FS
. Instead of invokingpython.mjs
directly, we will invokenode_entry.mjs
.One question on naming:
python.mjs
now cannot be used directly without some setup. Pyodide's naming convention calls thispyodide.asm.js
and the equivalent ofnode_entry.mjs
is calledpyodide.mjs
. So if we wanted to follow this same naming convention, we might renamepython.mjs
==>python.asm.mjs
andnode_entry.mjs
==>python.mjs
. I think what I've used here is fine though.Special care has to be taken to ensure that we get
sys.executable
correct. It should point to the original path that was invoked, without symlinks resolved. I updated the generatedpython.sh
to calculate it's own absolute path withrealpath -s $0
and pass that in. Thennode_entry.mjs
setsthisProgram
to the path passed in. I manually validated this by creating a symlink topython.sh
and invoking it.sys.executable
andsys._base_exectable
indeed point to the symlink, not to the build directorypython.sh
and not tonode_entry.mjs
.Pyodide solves this same problem by making the Python shell script into a node/bash polyglot. I think the approach I'm using here is better.
https://github.com/pyodide/pyodide/blob/main/src/templates/python
I haven't been able to track down how in the code
thisProgram
actually makes its way intosys.executable
, which is frustrating. But it is working in my tests and is the intended behavior by both Emscripten and Python.