Skip to content
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.

Sign up for GitHub

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

Jump to bottom

gh-126691: Remove --with-emscripten-target #126787

Merged
merged 3 commits into from
Nov 16, 2024
+38 −173
Merged

gh-126691: Remove --with-emscripten-target #126787

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
434f67f
gh-126691: Remove --with-emscripten-target
hoodmane Oct 3, 2024
4516f3e
Put back -sALLOW_MEMORY_GROWTH and -sEXIT_RUNTIME
hoodmane Nov 14, 2024
c11dae9
Update Misc/NEWS.d/next/Build/2024-11-13-15-47-09.gh-issue-126691.ni4…
hoodmane Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions Doc/using/configure.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,15 +454,6 @@ Options for third-party dependencies
WebAssembly Options
-------------------

.. option:: --with-emscripten-target=[browser|node]

Set build flavor for ``wasm32-emscripten``.

* ``browser`` (default): preload minimal stdlib, default MEMFS.
* ``node``: NODERAWFS and pthread support.

.. versionadded:: 3.11

.. option:: --enable-wasm-dynamic-linking

Turn on dynamic linking support for WASM.
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Build/2024-11-13-15-47-09.gh-issue-126691.ni4K-b.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Removed the ``--with-emscripten-target`` configure flag. We unified the
``node`` and ``browser`` options and same build can now be used independent
hoodmane marked this conversation as resolved.
Show resolved Hide resolved
of target runtime.
6 changes: 0 additions & 6 deletions Tools/wasm/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,6 @@ https://github.com/psf/webassembly for more information.

### Build

For now the build system has two target flavors. The ``Emscripten/browser``
target (``--with-emscripten-target=browser``) is optimized for browsers.
It comes with a reduced and preloaded stdlib without tests and threading
support. The ``Emscripten/node`` target has threading enabled and can
access the file system directly.

To cross compile to the ``wasm32-emscripten`` platform you need
[the Emscripten compiler toolchain](https://emscripten.org/),
a Python interpreter, and an installation of Node version 18 or newer. Emscripten
Expand Down
10 changes: 8 additions & 2 deletions Tools/wasm/emscripten/node_pre.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
// If process is undefined, we're not running in the node runtime let it go I
// guess?
if (typeof process !== "undefined") {
const nodeVersion = Number(process.versions.node.split('.',1)[0]);
const nodeVersion = Number(process.versions.node.split(".", 1)[0]);
if (nodeVersion < 18) {
process.stderr.write(`Node version must be >= 18, got version ${process.version}\n`);
process.stderr.write(
`Node version must be >= 18, got version ${process.version}\n`,
);
process.exit(1);
}
Module.preRun = () => {
FS.mkdirTree("/lib/");
FS.mount(NODEFS, { root: __dirname + "/lib/" }, "/lib/");
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this wasn't needed previously?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}
108 changes: 11 additions & 97 deletions configure
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

70 changes: 11 additions & 59 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1282,30 +1282,6 @@ AS_CASE([$ac_sys_system/$ac_sys_release],
]
)

AC_MSG_CHECKING([for --with-emscripten-target])
AC_ARG_WITH([emscripten-target],
[AS_HELP_STRING([--with-emscripten-target=@<:@browser|node@:>@], [Emscripten platform])],
[
AS_VAR_IF([ac_sys_system], [Emscripten], [
AS_CASE([$with_emscripten_target],
[browser], [ac_sys_emscripten_target=browser],
[node], [ac_sys_emscripten_target=node],
dnl Debug builds with source map / dwarf symbols. Py_DEBUG builds easily
dnl run out of stack space. Detached sybmols and map prohibit some
dnl optimizations and increase file size. Options are undocumented so we
dnl are free to remove them in the future.
[browser-debug], [ac_sys_emscripten_target=browser-debug],
[node-debug], [ac_sys_emscripten_target=node-debug],
[AC_MSG_ERROR([Invalid argument: --with-emscripten-target=browser|node])]
)
], [
AC_MSG_ERROR([--with-emscripten-target only applies to Emscripten])
])
], [
AS_VAR_IF([ac_sys_system], [Emscripten], [ac_sys_emscripten_target=browser])
])
AC_MSG_RESULT([$ac_sys_emscripten_target])

dnl On Emscripten dlopen() requires -s MAIN_MODULE and -fPIC. The flags
dnl disables dead code elimination and increases the size of the WASM module
dnl by about 1.5 to 2MB. MAIN_MODULE defines __wasm_mutable_globals__.
Expand Down Expand Up @@ -1350,10 +1326,9 @@ AC_ARG_WITH([suffix],
[EXEEXT=$with_suffix]
)
], [
AS_CASE([$ac_sys_system/$ac_sys_emscripten_target],
[Emscripten/browser*], [EXEEXT=.js],
[Emscripten/node*], [EXEEXT=.js],
[WASI/*], [EXEEXT=.wasm],
AS_CASE([$ac_sys_system],
[Emscripten], [EXEEXT=.js],
[WASI], [EXEEXT=.wasm],
[EXEEXT=]
)
])
Expand Down Expand Up @@ -1638,16 +1613,16 @@ AC_MSG_CHECKING([HOSTRUNNER])
AC_ARG_VAR([HOSTRUNNER], [Program to run CPython for the host platform])
if test -z "$HOSTRUNNER"
then
AS_CASE([$ac_sys_system/$ac_sys_emscripten_target],
[Emscripten/node*], [
AS_CASE([$ac_sys_system],
[Emscripten], [
AC_PATH_TOOL([NODE], [node], [node])
HOSTRUNNER="$NODE"
AS_VAR_IF([host_cpu], [wasm64], [AS_VAR_APPEND([HOSTRUNNER], [" --experimental-wasm-memory64"])])
],
dnl TODO: support other WASI runtimes
dnl wasmtime starts the process with "/" as CWD. For OOT builds add the
dnl directory containing _sysconfigdata to PYTHONPATH.
[WASI/*], [HOSTRUNNER='wasmtime run --wasm max-wasm-stack=16777216 --wasi preview2=n --env PYTHONPATH=/$(shell realpath --relative-to $(abs_srcdir) $(abs_builddir))/$(shell cat pybuilddir.txt):/Lib --dir $(srcdir)::/'],
[WASI], [HOSTRUNNER='wasmtime run --wasm max-wasm-stack=16777216 --wasi preview2=n --env PYTHONPATH=/$(shell realpath --relative-to $(abs_srcdir) $(abs_builddir))/$(shell cat pybuilddir.txt):/Lib --dir $(srcdir)::/'],
[HOSTRUNNER='']
)
fi
Expand All @@ -1660,10 +1635,8 @@ if test -n "$HOSTRUNNER"; then
fi

# LIBRARY_DEPS, LINK_PYTHON_OBJS and LINK_PYTHON_DEPS variable
AS_CASE([$ac_sys_system/$ac_sys_emscripten_target],
[Emscripten/browser*], [LIBRARY_DEPS='$(PY3LIBRARY) $(WASM_STDLIB) python.html python.worker.js'],
[LIBRARY_DEPS='$(PY3LIBRARY) $(EXPORTSYMS)']
)
LIBRARY_DEPS='$(PY3LIBRARY) $(EXPORTSYMS)'

freakboy3742 marked this conversation as resolved.
Show resolved Hide resolved
LINK_PYTHON_DEPS='$(LIBRARY_DEPS)'
if test "$PY_ENABLE_SHARED" = 1 || test "$enable_framework" ; then
LIBRARY_DEPS="\$(LDLIBRARY) $LIBRARY_DEPS"
Expand Down Expand Up @@ -2365,24 +2338,8 @@ AS_CASE([$ac_sys_system],
AS_VAR_APPEND([LDFLAGS_NODIST], [" -sUSE_PTHREADS"])
AS_VAR_APPEND([LINKFORSHARED], [" -sPROXY_TO_PTHREAD"])
])
AS_CASE([$ac_sys_emscripten_target],
[browser*], [
AS_VAR_IF([ac_sys_emscripten_target], [browser-debug], [wasm_debug=yes])
AS_VAR_APPEND([LINKFORSHARED], [" --preload-file=\$(WASM_ASSETS_DIR)"])
WASM_ASSETS_DIR=".\$(prefix)"
WASM_STDLIB="\$(WASM_ASSETS_DIR)/local/lib/python\$(VERSION)/os.py"
dnl separate-dwarf does not seem to work in Chrome DevTools Support.
WASM_LINKFORSHARED_DEBUG="-gsource-map --emit-symbol-map"
],
[node*], [
AS_VAR_IF([ac_sys_emscripten_target], [node-debug], [wasm_debug=yes])
AS_VAR_APPEND([LDFLAGS_NODIST], [" --pre-js=\$(srcdir)/Tools/wasm/emscripten/node_pre.js"])
AS_VAR_APPEND([LDFLAGS_NODIST], [" -sALLOW_MEMORY_GROWTH -sNODERAWFS"])
AS_VAR_APPEND([LINKFORSHARED], [" -sEXIT_RUNTIME"])
WASM_LINKFORSHARED_DEBUG="-gseparate-dwarf --emit-symbol-map"
]
)
AS_VAR_APPEND([LDFLAGS_NODIST], [" --pre-js=\$(srcdir)/Tools/wasm/emscripten/node_pre.js"])
WASM_LINKFORSHARED_DEBUG="-gseparate-dwarf --emit-symbol-map"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my own edification - the summary of this set of changes is that we can essentially use the node compilation options everywhere:

  • The --preload-file is not longer required;
  • The os.py replacement in the standard library is no longer required;
  • We don't need the -sALLOW_MEMORY_GROWTH, -sNODERAWFS and -sEXIT_RUNTIME flags any more
  • -gseparate-dwarf now presumably works in Chrome Devtools.

Copy link
Contributor Author

@hoodmane hoodmane Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 want atexit()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...

AS_VAR_IF([wasm_debug], [yes], [
AS_VAR_APPEND([LDFLAGS_NODIST], [" -sASSERTIONS"])
Expand Down Expand Up @@ -7466,12 +7423,7 @@ AC_MSG_CHECKING([for --disable-test-modules])
AC_ARG_ENABLE([test-modules],
[AS_HELP_STRING([--disable-test-modules], [don't build nor install test modules])], [
AS_VAR_IF([enable_test_modules], [yes], [TEST_MODULES=yes], [TEST_MODULES=no])
], [
AS_CASE([$ac_sys_system/$ac_sys_emscripten_target],
[Emscripten/browser*], [TEST_MODULES=no],
[TEST_MODULES=yes]
)
])
], [TEST_MODULES=yes])
AC_MSG_RESULT([$TEST_MODULES])
AC_SUBST([TEST_MODULES])

Expand Down
Loading