GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-40280: Add --enable-wasm-dynamic-linking (GH-32253) by tiran · Pull Request #32253 · python/cpython

Conversation

@tiran

@tiran tiran commented Apr 2, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

https://bugs.python.org/issue40280

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hoodmane and msabwat reacted with thumbs up emoji pmp-p reacted with heart emoji
All reactions
  • 👍 2 reactions
  • ❤️ 1 reaction
@bedevere-bot bedevere-bot added the awaiting core review label Apr 2, 2022
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Apr 2, 2022
@tiran
tiran requested review from brettcannon and erlend-aasland April 2, 2022 15:53
@tiran
tiran force-pushed the bpo-40280-wasm-dynamic-linking branch from 7cfc9d8 to ad6a484 Compare April 2, 2022 15:54
@tiran tiran mentioned this pull request Apr 2, 2022
Review CPython patches / tests and contribute upstream pyodide/pyodide#2000
Closed
pmp-p
pmp-p reviewed Apr 2, 2022
View reviewed changes
Comment thread configure Resolved

Uh oh!

There was an error while loading. Please reload this page.

@pmp-p pmp-p mentioned this pull request Apr 2, 2022
Get ctypes working emmatyping/python-wasm#68
Open
@hoodmane

hoodmane commented Apr 2, 2022

Copy link
Copy Markdown
Contributor

Maybe this should be called --enable-emscripten-dynamic-linking since it doesn't work with wasi or other wasm targets?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran

tiran commented Apr 2, 2022

Copy link
Copy Markdown
Member Author

Maybe this should be called --enable-emscripten-dynamic-linking since it doesn't work with wasi or other wasm targets?

WASI does not support dynamic linking yet. It may get it in the future, https://helda.helsinki.fi/bitstream/handle/10138/337740/Dynamic_linking_in_WebAssembly_Architecture_and_Performance_Evaluation.pdf .

hoodmane and pmp-p reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hoodmane

hoodmane commented Apr 2, 2022

Copy link
Copy Markdown
Contributor

But probably stuff like -s SIDE_MODULE=1 will always be emscripten specific, so even when other wasm platforms gain dynamic linking it is not clear if this flag can work for both.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hoodmane

hoodmane commented Apr 2, 2022

Copy link
Copy Markdown
Contributor

I still think we should discuss with Emscripten whether we could set some environment variable like EMSCRIPTEN_RESPECT_SHARED and then use the -shared flag. Then other wasm targets would just ignore the EMSCRIPTEN_RESPECT_SHARED variable, whereas they would probably get angry when passed nonsense like -s SIDE_MODULE=1 as a command line argument.

cf emscripten-core/emscripten#16481

@sbc100

pmp-p reacted with thumbs up emoji pmp-p reacted with heart emoji
All reactions
  • 👍 1 reaction
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran

tiran commented Apr 2, 2022

Copy link
Copy Markdown
Member Author

I still think we should discuss with Emscripten whether we could set some environment variable like EMSCRIPTEN_RESPECT_SHARED and then use the -shared flag. Then other wasm targets would just ignore the EMSCRIPTEN_RESPECT_SHARED variable, whereas they would probably get angry when passed nonsense like -s SIDE_MODULE=1 as a command line argument.

WASI wouldn't see the Emscripten flags. We can easily extend our configure file later to do something like:

AS_VAR_IF([enable_wasm_dynamic_linking], [yes], [
  AS_CASE([$ac_sys_system],
    [Emscripten], [BLDSHARED='$(CC) -shared -s SIDE_MODULE=1 -s WASM=1'],
    [WASI], [BLDSHARED='$(CC) --shared --wasi-magic-linking-flag']
  )
])
hoodmane reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hoodmane

hoodmane commented Apr 2, 2022

Copy link
Copy Markdown
Contributor

Right, makes sense that autotools has ways of dealing with different compilers needing different flags.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pmp-p
pmp-p approved these changes Apr 2, 2022
View reviewed changes
erlend-aasland
erlend-aasland approved these changes Apr 2, 2022
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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.

AC changes look good to me!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread configure.ac Resolved

Uh oh!

There was an error while loading. Please reload this page.

tiran added 5 commits April 3, 2022 18:35
@tiran
bpo-40280: Add --enable-wasm-dynamic-linking
dbc74ad
@tiran
Set to missing, add breadcrumbs for WASI
9d5b67a
@tiran
Typo
17998ea
@tiran
Tab vs. space
09e5ecf
@tiran
Drop space between -s and argument
c1a4260
@tiran
tiran force-pushed the bpo-40280-wasm-dynamic-linking branch from e03d117 to c1a4260 Compare April 3, 2022 17:25
@tiran

tiran commented Apr 3, 2022

Copy link
Copy Markdown
Member Author

The PR drops ac_cv_func_dlopen=no from our config.site override and will allow us to test dynamic linking in the future.

@hoodmane does the PR help you, or at least not cause any new problems?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hoodmane

hoodmane commented Apr 3, 2022

Copy link
Copy Markdown
Contributor

I'm having trouble testing on tip of tree because of the changes to _sre and regex. I need a docker image like 3.11.0a6-slim-buster but with tip of tree. Or maybe I can just locate the changes to _sre and regex since alpha6 and revert them? I can still build locally against a locally built copy of tot Python but it's inconvenient not to have the CI working.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hoodmane

hoodmane commented Apr 3, 2022

Copy link
Copy Markdown
Contributor

Or maybe I can cherry-pick this commit onto 3.11.0a6?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran

tiran commented Apr 3, 2022

Copy link
Copy Markdown
Member Author

What's the problem with _sre module? GH-32177 converted the modules to a package. The change should be fully backwards compatible.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hoodmane

hoodmane commented Apr 3, 2022

Copy link
Copy Markdown
Contributor

I get failures at
assert _sre.MAGIC == MAGIC, "SRE module mismatch" on line 17 of Lib/sre_compile.py

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran

tiran commented Apr 3, 2022

Copy link
Copy Markdown
Member Author

There is no line 17 in Lib/sre_compile.py any more. The file got moved and replaced with a shorter stub. Some files on your system are out of date.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hoodmane

hoodmane commented Apr 3, 2022

Copy link
Copy Markdown
Contributor

Some files on your system are out of date.

Yeah hence I need a docker image with tip of tree Python because my v3.11.0a6 system is out of date

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran

tiran commented Apr 3, 2022

Copy link
Copy Markdown
Member Author

Are you familiar with out-of-tree builds? We compile a build Python interpreter to bootstrap Emscripten cross build from the same source check, https://github.com/ethanhs/python-wasm/blob/main/build-python-build.sh and https://github.com/ethanhs/python-wasm/blob/main/build-python-emscripten-browser.sh . The trick is ../../configure.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hoodmane

hoodmane commented Apr 3, 2022

Copy link
Copy Markdown
Contributor

Are you familiar with out-of-tree builds?

Apparently not.

The trick is ../../configure

Is it important that there are two layers builddir/build?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hoodmane

hoodmane commented Apr 3, 2022

Copy link
Copy Markdown
Contributor

Also, are there docs on out of tree build somewhere that I should read?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran

tiran commented Apr 3, 2022

Copy link
Copy Markdown
Member Author

No, the paths are not important. Any directory structure will do. It's only important that you run make clean in the source directory first. And don't use build, it gets easily confused with distutils's build directory. ouf-of-tree builds (aka VPATH builds) are an autoconf feature. https://www.gnu.org/software/automake/manual/html_node/VPATH-Builds.html

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hoodmane

hoodmane commented Apr 3, 2022

Copy link
Copy Markdown
Contributor

It's only important that you run make clean in the source directory first.

Do I need to do this if I have cloned a fresh copy of python/cpython?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran

tiran commented Apr 3, 2022

Copy link
Copy Markdown
Member Author

Do I need to do this if I have cloned a fresh copy of python/cpython?

No, a fresh clone is clean.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hoodmane

hoodmane commented Apr 3, 2022

Copy link
Copy Markdown
Contributor

I tried to follow your directions for this:
https://github.com/hoodmane/pyodide/blob/python-tot/cpython/Makefile
But the build still fails with:

Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/src/cpython/installs/python-3.11.0dev0/lib/python3.11/lib2to3/pgen2/driver.py", line 21, in <module>
    import logging
    ^^^^^^^^^^^^^^
  File "/src/cpython/installs/python-3.11.0dev0/lib/python3.11/logging/__init__.py", line 26, in <module>
    import sys, os, time, io, re, traceback, warnings, weakref, collections.abc
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/src/cpython/installs/python-3.11.0dev0/lib/python3.11/re/__init__.py", line 125, in <module>
    from . import _compiler, _parser
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/src/cpython/installs/python-3.11.0dev0/lib/python3.11/re/_compiler.py", line 17, in <module>
    assert _sre.MAGIC == MAGIC, "SRE module mismatch"
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: SRE module mismatch

Maybe you can tell me what I'm doing wrong?
https://app.circleci.com/pipelines/github/hoodmane/pyodide/2654/workflows/282f60fd-a7e5-4d22-8688-09f2daddcf03/jobs/28765

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hoodmane

hoodmane commented Apr 3, 2022

Copy link
Copy Markdown
Contributor

Ah maybe the problem is just buggy make recipes.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hoodmane

hoodmane commented Apr 3, 2022

Copy link
Copy Markdown
Contributor

I think the_sre.MAGIC crash is triggered while it is trying to log some other error, something about pybuilddir.txt.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hoodmane

hoodmane commented Apr 3, 2022

Copy link
Copy Markdown
Contributor

Okay the problem is on Makefile.pre.in line 2032:

	-PYTHONPATH=$(DESTDIR)$(LIBDEST)  $(RUNSHARED) \
		$(PYTHON_FOR_BUILD) -Wi $(DESTDIR)$(LIBDEST)/compileall.py \
		-j0 -d $(LIBDEST) -f \
		-x 'bad_coding|badsyntax|site-packages|lib2to3/tests/data' \
		$(DESTDIR)$(LIBDEST)

Note that this is the build Python (PYTHON_FOR_BUILD) running with the target Python's standard library (-PYTHONPATH=$(DESTDIR)$(LIBDEST)).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hoodmane

hoodmane commented Apr 3, 2022

Copy link
Copy Markdown
Contributor

Or something. I tried manually removing PYTHONPATH from that and it still broke in the same way so I don't know.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran

tiran commented Apr 4, 2022

Copy link
Copy Markdown
Member Author

Is the error coming from libinstall target? Our python-wasm PoC does not use make install. We are building WASM port in-place and then copy files around manually. I'll investigate after next alpha is out.

I'm merging the PR now to get it into the upcoming alpha. It's unlike to cause you any problems and allows us to test dynamic linking more easily.

hoodmane reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran tiran changed the title bpo-40280: Add --enable-wasm-dynamic-linking bpo-40280: Add --enable-wasm-dynamic-linking (GH-32253) Apr 4, 2022
@tiran
tiran merged commit c9844cb into python:main Apr 4, 2022
@tiran
tiran deleted the bpo-40280-wasm-dynamic-linking branch April 4, 2022 17:31
@bedevere-bot bedevere-bot removed the awaiting core review label Apr 4, 2022
@hoodmane

hoodmane commented Apr 4, 2022

Copy link
Copy Markdown
Contributor

Yeah I think make libinstall has a bug where when cross compiling it uses the Python standard library of the target Python with the Python executable of the build Python. As long as the target Python and the build Python are compatible for the small number of packages it needs, it works okay. But if there are changes to certain libraries, it breaks.

pmp-p reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran

tiran commented Apr 5, 2022

Copy link
Copy Markdown
Member Author

Congratulations, you have discovered an annoying issue with development during Python's alpha phase. :)

Code is moving fast and breaks often. You should rebuild your build Python interpreter every time you update your checkout or switch branches. It's going to stabilize during beta and won't be an issue once Python 3.11 reaches RC and final stages. I recommend config cache (./configure -C) and ccache (export EM_COMPILER_WRAPPER=ccache + export PATH=/usr/lib/ccache:$PATH) to speed up configure and build steps.

erlend-aasland and hoodmane reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hoodmane

hoodmane commented Apr 5, 2022

Copy link
Copy Markdown
Contributor

Makes sense. At least Python is fast to build. The main annoyance is that if I want to get the CI running I have to set up building Python either in my Dockerfile or in my pipeline.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hoodmane

hoodmane commented Apr 5, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Hmm, I'm still having trouble. Even building a x86 Linux Python and passing it as --with-build-python=, the build is generating invocations to /usr/local/bin/python3.11. The issue seems to be that ./configure is generating a slightly buggy Makefile. A bunch of variables are all on the same line:

PYTHON_FOR_BUILD=_PYTHON_PROJECT_BASE=$(abs_builddir) _PYTHON_HOST_PLATFORM=$(_PYTHON_HOST_PLATFORM) PYTHONPATH=$(shell test -f pybuilddir.txt && echo $(abs_builddir)/`cat pybuilddir.txt`:)$(srcdir)/Lib _PYTHON_SYSCONFIGDATA_NAME=_sysconfigdata_$(ABIFLAGS)_$(MACHDEP)_$(MULTIARCH) /src/cpython/build/Python-3.11.0dev0/builddir/build-host/python

I think this should be:

PYTHON_FOR_BUILD=/src/cpython/build/Python-3.11.0dev0/builddir/build-host/python
_PYTHON_PROJECT_BASE=$(abs_builddir) 
_PYTHON_HOST_PLATFORM=$(_PYTHON_HOST_PLATFORM) 
PYTHONPATH=$(shell test -f pybuilddir.txt && echo $(abs_builddir)/`cat pybuilddir.txt`:)$(srcdir)/Lib 
_PYTHON_SYSCONFIGDATA_NAME=_sysconfigdata_$(ABIFLAGS)_$(MACHDEP)_$(MULTIARCH)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brettcannon

brettcannon commented Apr 5, 2022

Copy link
Copy Markdown
Member

@hoodmane at this point it's probably better to open an issue, else this is liable to get lost since the PR has already been merged.

pmp-p reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hoodmane

hoodmane commented Apr 5, 2022

Copy link
Copy Markdown
Contributor

Okay I opened an issue here: https://bugs.python.org/issue47232

pmp-p reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sbc100
sbc100 reviewed Apr 13, 2022
View reviewed changes
Comment thread configure.ac
dnl Emscripten's emconfigure sets LDSHARED. Set BLDSHARED outside the
dnl test -z $LDSHARED block to configure BLDSHARED for side module support.
if test "$enable_wasm_dynamic_linking" = "yes" -a "$ac_sys_system" = "Emscripten"; then
BLDSHARED='$(CC) -shared -sSIDE_MODULE=1 -sWASM=1'

@sbc100 sbc100 Apr 13, 2022

Copy link
Copy Markdown

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.

Is there some reason why you add -sWASM=1 here. Since its the default its unlikely you need it.

Also, you can simplify this command line (and the ones elsewhere in this file) by dropping with =1 at the end of all the settings.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pmp-p reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@tiran tiran Apr 13, 2022

Copy link
Copy Markdown
Member Author

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.

Python shared extensions have a .so extension. emcc creates a .so JavaScript file and a .wasm file along the .so file. We want the .so file to contain the .wasm code.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@sbc100 sbc100 Apr 13, 2022

Copy link
Copy Markdown

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 not what the -sWASM=1 option does though. The -sSIDE_MODULE option is enough to ensure that the output is just a wasm file.

If you are building a normal application without -sSIDE_MODULE then you always get a JS file and a wasm file side-by-side (even with -sWASM=1). The -sWASM=1 option (which is the default) simply means "don't convert the wasm file to JS using wasm2js".. which is what happens if you set -sWASM=0 (which is never the default).

-sSIDE_MODULE on its own should always build just a wasm file.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hoodmane and pmp-p reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

@brettcannon brettcannon Apr 14, 2022

Copy link
Copy Markdown
Member

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.

Please note this PR is merged already, so it would be best to open a new issue to track this discussion if there's something to change.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jakirkham reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@SimonBiggs SimonBiggs mannequin mentioned this pull request Jun 7, 2022
Consider supporting emscripten/webassembly as a build target #84461
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hoodmane hoodmane hoodmane left review comments
@erlend-aasland erlend-aasland erlend-aasland approved these changes
@brettcannon brettcannon Awaiting requested review from brettcannon
+2 more reviewers
@sbc100 sbc100 sbc100 left review comments
@pmp-p pmp-p pmp-p approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@tiran @hoodmane @brettcannon @sbc100 @erlend-aasland @pmp-p @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.