cibuildwheel icon indicating copy to clipboard operation
cibuildwheel copied to clipboard

Add Emscripten support

Open hoodmane opened this issue 2 years ago • 34 comments

Resolves #1454. This is on top of #1462. Also close #1002.

We have trouble with the tests because currently python -m pytest is required (pytest by itself doesn't work as expected). Also python -m pip does not work, have to use pip. It's a work in progress...

TODO:

  • Automate Python installation in certain CI environments?

Working:

  • [x] multidict -- (requires this fix to their config)
  • [ ] numpy -- builds wheel but hangs 99% of the way through test suite
  • [x] scikitlearn
  • [x] scikit-image -- works on v0.19.3, doesn't work on v0.20 because of Meson
  • [ ] mypy -- NonPlatformWheelError
  • [x] ultrajson -- one test failure due to fork
  • [ ] ril -- rust compilation errors
  • [x] pydantic -- one test failure due to asyncio.run

hoodmane avatar Apr 04 '23 22:04 hoodmane

@henryiii could you approve the github actions workflows?

hoodmane avatar Apr 05 '23 20:04 hoodmane

@henryiii is the windows_x86 failure a flake?

failed to download pip version 20.0.2, pip download exit code 1
The conflict is caused by:
    The user requested pip==20.0.2
    The user requested (constraint) pip==20.0.2
To fix this you could try to:
1. loosen the range of package versions you've specified
2. remove package versions to allow pip attempt to solve the dependency conflict
WARNING: Skipping page https://pypi.org/simple/pip/ because the GET request got Content-Type: .The only supported Content-Type is text/html
ERROR: Cannot install pip==20.0.2 because these package versions have conflicting dependencies.
ERROR: ResolutionImpossible: for help visit https://pip.pypa.io/en/latest/user_guide/#fixing-conflicting-dependencies

hoodmane avatar Apr 07 '23 17:04 hoodmane

@henryiii is the windows_x86 failure a flake?

Yes. Some networking issue that is hard to track down.

henryiii avatar Apr 27 '23 23:04 henryiii

BTW, would you like me to rebase for you? I don't mind.

henryiii avatar May 01 '23 04:05 henryiii

I already merged main locally.

hoodmane avatar May 01 '23 04:05 hoodmane

Okay I think this is working now. Would appreciate a review @henryiii, @joerick

hoodmane avatar May 06 '23 15:05 hoodmane

Presumably we need documentation somewhere for the fact that Python 311 needs to be installed.

hoodmane avatar May 06 '23 15:05 hoodmane

Splendid work on this so far @hoodmane ! Quality looks really good. Comments above.

joerick avatar May 12 '23 10:05 joerick

We talked about the Python requirement at PyCon a bit - I think it's best to require 3.11 instead of adding a lot of time & bandwidth for a docker image. Since pyodide is a bit behind official CPython, all CI's should have a suitable host Python available. If PyBI's land, then we might be able to make this simpler. :)

I think the name is a bit in the air. Technically, it's emscripten + version + flags. But pyodide selects a specific set, and the hope is it won't update between Python versions. So 3.12 would be a new version of emscripten + flag updates, etc. But I think that's going to be incorrect at least once (I think there's a planned 3.11 update). The hope is by the time wheels are cleared for PyPI, it will be stable. There's also the possibility that CPython will move emscripten to tier 2 (not soon, sadly, looks like only WASI is going to tier 2), in which case CPython could take over the definition of the emscripten version + flags for whatever version it started on.

This will need docs - it should be clear this is experimental and you can't upload the wheels to PyPI yet. Setting it up is a bit more involved than just adding it to a matrix, etc. I can help with docs later (traveling at conference this week).

Personally and quite selfishly, I really want the cmake bug fixed before it ships, I expect scikit-build/scikit-build-core users will want to make pyodide wheels once this ships in cibuildwheel, and can't do that due to a variable being stripped from CMake's environment by emscripten and pyodide isn't auto-fixing out-of-tree wheels. I can help fix this early next week.

henryiii avatar May 12 '23 14:05 henryiii

Thanks for the detailed review @joerick!

hoodmane avatar May 12 '23 21:05 hoodmane

pyodide isn't auto-fixing out-of-tree wheels

You're talking about replace_so_abi_tags()? I think we can release a Pyodide 0.23.3 that calls that for out of tree wheels.

@ryanking13 do you see any reason not to call replace_so_abi_tags() when building out of tree? This probably fits into your project of making the out of tree builds more the same as the in-tree ones.

Of course it would be great if we could also fix the issue with Emscripten, but it seems potentially easier to change Pyodide. https://github.com/emscripten-core/emscripten/pull/19301

hoodmane avatar May 12 '23 21:05 hoodmane

@ryanking13 do you see any reason not to call replace_so_abi_tags() when building out of tree? This probably fits into your project of making the out of tree builds more the same as the in-tree ones.

Sounds good. We should try to make out of tree and in tree builds identical.

ryanking13 avatar May 13 '23 00:05 ryanking13

You're talking about replace_so_abi_tags()?

There are three fixes, any one of them would be enough:

  1. Run replace_so_abi_tags() for out-of-tree builds. It's a bit of a hack, but it's a minimum to get things working.
  2. Replace emcmake with pyodide's own implementation. This has to be pretty easy, it's a really simple script. This would allow scikit-build-core to get the extension right to begin with, and would support FindPython properly I think.
  3. Fix emscripten to not strip this variable when using emcmake. That's a harder fix, and would require an upstream change.

I'd really like both 1 and 2 if possible. :)

henryiii avatar May 15 '23 20:05 henryiii

Okay I think all the integration tests are passing and I think I addressed the most recent round of review comments. So this is waiting on:

  • [x] review round 2?
  • [x] fix the so_abi problems

@henryiii I'll make a PR to run replace_so_abi_tags for out of tree builds, you can work on an emcmake replacement. Then we can release pyodide 0.23.3 and update cibuildwheel to use it. Then we can merge this (assuming that I address any further review comments).

hoodmane avatar May 15 '23 21:05 hoodmane

What's the current status of this work? Is there any chance to merge it with an experimental status in the near future (once CI passes) and then iron out the potential build issues for individual packages as they come along?

For instance, I would really love to be able to do this for building dev scikit-learn wheels, which already work according to the issue description.

rth avatar Jun 22 '23 08:06 rth

It needs to be updated for the recent pyodide release & rebased. Since it's not "default" (that is, you have to ask for the platform if you want it), I think it's pretty safe to add.

henryiii avatar Jun 22 '23 13:06 henryiii

(PS: I'm happy to do the update & rebase, but don't want to force push something if @hoodmane is already working on it - just let me know)

henryiii avatar Jun 22 '23 13:06 henryiii

Ahh, there are 75 commits & multiple merges. Guess I'll just merge too.

henryiii avatar Jun 22 '23 15:06 henryiii

I'm curious to see if this fails CI. I know there's a bug which I've just fixed locally but would like to see if anything in CI catches it.

henryiii avatar Jun 22 '23 16:06 henryiii

Thanks @henryiii !

rth avatar Jun 22 '23 16:06 rth

Okay, "no space left on device" wasn't the failure I was expecting...

henryiii avatar Jun 22 '23 18:06 henryiii

@hoodmane, do you know why tests are failing to set up a venv?

FWIW, I'm testing it locally with awkward-array like this:

docker run -v $PWD:/cibuildwheel -v $PWD/../../scikit-hep/awkward:/awkward -w /awkward/awkward-cpp --rm -it python:3.11 bash
pip install /cibuildwheel
cibuildwheel --platform pyodide

And I'm getting:

FAILED: _ext.cpython-311-x86_64-linux-gnu.so
: && /tmp/tmp5ii37twm/c++ -fPIC -std=c++14 -std=c++14 -std=c++14 -std=c++14 -s DISABLE_EXCEPTION_CATCHING=0 -O3 -DNDEBUG  -flto -shared  -o _ext.cpython-311-x86_64-linux-gnu.so CMakeFiles/_ext.dir/src/python/_ext.cpp.o CMakeFiles/_ext.dir/src/python/content.cpp.o CMakeFiles/_ext.dir/src/python/forth.cpp.o CMakeFiles/_ext.dir/src/python/io.cpp.o  libawkward-static.a  libawkward-cpu-kernels-static.a && cd /awkward/awkward-cpp/build/cpython-311 && emstrip /awkward/awkward-cpp/build/cpython-311/_ext.cpython-311-x86_64-linux-gnu.so
Traceback (most recent call last):
  File "/tmp/tmp5ii37twm/c++", line 668, in <module>
    compiler_main()
  File "/tmp/tmp5ii37twm/c++", line 664, in compiler_main
    sys.exit(handle_command(args, build_args))
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/tmp5ii37twm/c++", line 654, in handle_command
    returncode = subprocess.run(new_args).returncode
                 ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/subprocess.py", line 548, in run
    with Popen(*popenargs, **kwargs) as process:
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/subprocess.py", line 1024, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/local/lib/python3.11/subprocess.py", line 1917, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
OSError: [Errno 7] Argument list too long: 'em++'
[174/174] Linking CXX shared library libawkward.so
em++: warning: EXPORTED_FUNCTIONS is not valid with LINKABLE set (normally due to SIDE_MODULE=1/MAIN_MODULE=1) since all functions are exported this mode.  To export only a subset use SIDE_MODULE=2/MAIN_MODULE=2 [-Wunused-command-line-argument]
em++: warning: ignoring unsupported linker flag: `-soname` [-Wlinkflags]
ninja: build stopped: subcommand failed.

(This works outside of cibuildwheel on awkward-array, by the way, now that 0.23.3 is out)

henryiii avatar Jun 22 '23 19:06 henryiii

OSError: [Errno 7] Argument list too long: 'em++'

Seems to be the same issue as in https://github.com/pyodide/pyodide/issues/3427#issuecomment-1374418990

rth avatar Jun 22 '23 19:06 rth

Ahh, yes, that's there in: https://github.com/scikit-hep/awkward/blob/2d259cc54e3787f1d285a57274e5ce5748ff110f/.github/workflows/docs.yml#L112

Not sure how I'm supposed to pass that with cibuildwheel, though.

henryiii avatar Jun 22 '23 19:06 henryiii

@hoodmane would adding an environment variable version of --exports whole_archive be a good idea for 0.23.4?

henryiii avatar Jul 01 '23 13:07 henryiii

I think we could do that.

hoodmane avatar Jul 01 '23 13:07 hoodmane

Hmm, so the CI now fails building the pp310-manylinux_i686 wheel, unless I misread the logs. Not very clear to me what's the actual error..

rth avatar Jul 18 '23 05:07 rth

The error was "No space left", this should now be fixed in main.

mayeut avatar Aug 17 '23 23:08 mayeut

pyodide updated its version to 0.24 on 13-September-2023...

celestinoxp avatar Sep 16 '23 22:09 celestinoxp

I merged with main to update this to the new build-frontend option.

joerick avatar Sep 19 '23 11:09 joerick