Better package cross compilation by hoodmane · Pull Request #2238 · pyodide/pyodide

hoodmane · 2022-03-02T07:11:41Z

Instead of trying to convince setup.py to do what we want, the wheel
package empowers us to unpack and directly manipulate the wheel.
In particular this is important because --skip-build really does
not work.

First we build a native wheel. If skip_host is True, then this
"native" wheel has defective empty files as binaries, if it's False,
we build a real native wheel. If skip_host is False, then we use
pip to install our native wheel into host directory.

Then we unpack the wheel, replace the platform tag with
wasm32_emscripten, remove the non-metadata files, and copy in the
new files with the cross compiled binaries.

Of course, this is all only necessary for binary extensions.

Hopefully this will help get us towards using alternative build
backends. The one concern is that this approach depends on
implementation details of where the build backend puts the files
into the directory tree before copying them into the wheel. If
the build backend moved the linked binaries after building them,
we would be in trouble. Or worse if the build backend deleted the
files after packing them into the wheel, we'd be in trouble.

Basically, it seems clear that we can only support alternative
build backends on a case-by-case basis.

@henryiii I'm curious what your thoughts are cross compilation + arbitrary build backends.

We also update it to handle .so files that are not in the MEMFS. (In this case we have to copy the file into memory before loading it.)

Instead of trying to convince setup.py to do what we want, the wheel package empowers us to unpack and directly manipulate the wheel. In particular this is important because `--skip-build` really does not work. First we build a native wheel. If skip_host is True, then this "native" wheel has defective empty files as binaries, if it's False, we build a real native wheel. If skip_host is False, then we use pip to install our native wheel into host directory. Then we unpack the wheel, replace the platform tag with wasm32_emscripten, remove the non-metadata files, and copy in the new files with the cross compiled binaries. Of course, this is all only necessary for binary extensions. Hopefully this will help get us towards using alternative build backends. The one concern is that this approach depends on implementation details of where the build backend puts the files into the directory tree before copying them into the wheel. If the build backend moved the linked binaries after building them, we would be in trouble. Or worse if the build backend deleted the files after packing them into the wheel, we'd be in trouble. Basically, it seems clear that we can only support alternative build backends on a case-by-case basis.

This reverts commit 53900b0.

henryiii · 2022-03-03T15:38:29Z

I'd have to understand the cross-compile process a little better. Where is the "cross-compiled" run being done? I'm guessing it's picking up the compile commands, then reprocessing them, or something like that?

hoodmane · 2022-03-03T17:00:45Z

I'm guessing it's picking up the compile commands, then reprocessing them, or something like that?

Yes. On this branch, we run python setup.py bdist_wheel with symlinks installed so that attempts to invoke the C compiler toolchain are recorded into $pkg_root/build/$src_dir/build.log. If skip_host is not false, we just emit an empty output file rather than actually invoking the compiler. Then we delete these "native" artifacts.
This is done by the pywasmcross.capture_compile function. capture_make_command_wrapper_symlinks places the symlinks into the path.

Then we replay the compilation with pywasmcross.replay_compile which iterates over build.log and calls replay_command, which does some rewrite process to remove flags that don't make sense or replace them with other flags, rearrange which libraries are loaded, remove or reroute system libraries or includes into emscripten ones etc. (This is quite similar to what happens in emcc.py which is after a lot of the same stuff.)

Then we unpack the wheel, copy the new binary files into the wheel, rewrite the platform tag, and repack.

pyodide-build/pyodide_build/buildpkg.py

ryanking13

Thanks, @hoodmane! Generally LGTM.

It would be great if rth can review this too, as I haven't reviewed our build system very much. But he is doing more important things so I'm fine with merging this as there are other PRs waiting for this. (We can discuss our build system more in #2256 anyway)

packages/scipy/meta.yaml

Co-authored-by: Gyeongjae Choi <[email protected]>

hoodmane · 2022-03-10T01:33:52Z

Thanks for the review @ryanking13!

I haven't reviewed our build system very much

It's very confusing and we have a steep learning curve, but thankfully we seem to be gradually increasing how much we understand the build system. Probably it would be good to devote some time to improving the dev docs or in line comments on this topic to reduce the confusingness.

I'm fine with merging this as there are other PRs waiting for this

To be clear, my opinion is that this replaces the hack we already have with a new hack which is marginally simpler and more predictable. But this is enough to allow us to update to Python 3.10 =)

#2256 will dramatically improve predictability and get us away from being tied to setuptools. It also removes slightly more than 400 loc on net.

hoodmane · 2022-03-10T04:34:22Z

Okay I guess I'll merge this then.

Co-authored-by: Gyeongjae Choi <[email protected]> Instead of trying to convince setup.py to do what we want, the wheel package empowers us to unpack and directly manipulate the wheel. In particular this is important because --skip-build really does not work. First we build a native wheel. If skip_host is True, then this "native" wheel has defective empty files as binaries, if it's False, we build a real native wheel. If skip_host is False, then we use pip to install our native wheel into host directory. Then we unpack the wheel, replace the platform tag with wasm32_emscripten, remove the non-metadata files, and copy in the new files with the cross compiled binaries. Of course, this is all only necessary for binary extensions. Hopefully this will help get us towards using alternative build backends. The one concern is that this approach depends on implementation details of where the build backend puts the files into the directory tree before copying them into the wheel. If the build backend moved the linked binaries after building them, we would be in trouble. Or worse if the build backend deleted the files after packing them into the wheel, we'd be in trouble. Basically, it seems clear that we can only support alternative build backends on a case-by-case basis.

@henryiii

Our package build process currently has a significant flaw: we first run setup.py, recording all compilation commands, then we rewrite these compilation commands to invoke emcc and replay them, and then we pray that the cross compiled executables ended up in the right place to go into the wheel. This is not a good strategy because the build script is allowed to implement arbitrary logic, and if it moves, renames, etc any of the output files then we lose track of them. This has repeatedly caused difficulty for us. However, we also make no particularly significant use of the two pass approach. We can just do the simpler thing: capture the compiler commands as they occur, modify them as needed, and then run the fixed command. I also added a patch to fix the numpy feature detection for wasm so that we don't have to include _npyconfig.h and config.h, numpy can generate them in the way it would for a native build. I opened a numpy PR that would fix the detection for us upstream: numpy/numpy#21154 This clears the way for us to switch to using pypa/build (as @henryiii has suggested) by removing our dependence on specific setuptools behavior. This is on top of #2238.

rth

Thanks @hoodmane !

I agree it's a good idea to get rid of setup.py install there, and the general direction sounds good to me. This does depend on some of the implementation details, as you mentionned, but we have a wide enough variety of packages to catch most common issues I think. So as long as it builds it should be OK.

A few minor comments below, mostly about docs (feel fee to aggregate them into a single PR, I'm going to have a look at some other recent PRs as well).

rth · 2022-03-18T10:37:57Z

pyodide-build/pyodide_build/buildpkg.py

+    WHEEL.write_text("\n".join(result_lines))
+
+
+def update_wheel_platform(build_dir: Path, wheel_dir: Path):


Would you mind adding a docstring explaing what exactly we are updating and where?

I think this function was removed in #2256.

rth · 2022-03-18T10:41:25Z

pyodide-build/pyodide_build/buildpkg.py

-    del bash_runner.env["_PYTHON_HOST_PLATFORM"]
+
+
+def set_host_platform(wheel_dir: Path, platform: str):


Same, a docstring with a sentence saying that this edits *.dist-info/WHEEL to replace the Tag value with the platform, and linking to the PEP or documentation listing valid values there wouldn't hurt.

Same, removed in #2256

rth · 2022-03-18T10:46:33Z

pyodide-build/pyodide_build/pywasmcross.py

+                    sys.executable,
+                    "-m",
+                    "pip",
+                    "install",


The only thing is pip will often use build isolation to install build requirements from pyproject.toml and not whatever dependencies we indicated in our build system.
In most cases, that's probably fine, but I wonder if this won't be an issue if the build dependency is patched. Say you build a package that needs numpy at build time, and it will use a different (and unpatched) numpy version to install it on the host when skip_host=False.
Again I'm not sure if it's really an issue, but we could at least mention it in a comment that in some cases it might be appropriate to disable build isolation here.

Oh yeah, this was also removed in #2256 (along with the entire skip_host key). Only numpy was setting skip_host to false too, so if there were problems on this code path we probably wouldn't know.

hoodmane added 3 commits March 1, 2022 18:25

Fix loadDynlib to handle files with length < capacity.

87fd359

We also update it to handle .so files that are not in the MEMFS. (In this case we have to copy the file into memory before loading it.)

Remove skip-build patch from swiglpk since it's no longer needed

1f71ce6

This was referenced Mar 2, 2022

Update lxml #2239

Merged

Python 3.10 #2225

Merged

hoodmane added 8 commits March 2, 2022 00:30

Fix numpy build

881dfaf

Fix PYTHONPATH

251100e

Fix pythonpath in pyodide-build

93e060f

Update restore_cache to include fallback

53900b0

Revert makefile.envs

f31eef0

Revert "Update restore_cache to include fallback"

49429ab

This reverts commit 53900b0.

Temporarily remove scipy and dependent packages

1b34e69

Modify Makefile.envs again

416104b

Invalidate cache

c890ba0

hoodmane added 8 commits March 3, 2022 09:55

Fix NUMPY_ABI_VERSION and NUMPY_API_VERSION

6a537dd

Try again with fixing numpy build

4ffead9

Fix numpy build script more

b06272b

Merge branch 'main' into better-cross-build

c65cf4e

Add Scipy back in

4b0da26

Put back other packages

bb61b14

Increase timeout for build packages

40c07db

Try restoring old cache

df074e7

hoodmane mentioned this pull request Mar 4, 2022

MAINT use variables for Python version rather than hardcoding it #2241

Merged

hoodmane added 5 commits March 4, 2022 00:45

Fix scipy build

1c69b42

Finish fixing scipy build

b36e202

Revert changes to circleci config

0a79075

Merge branch 'main' into better-cross-build

71a07ce

Try to fix build

65cad06

Use '--prefix' instead of '--root'

ad7c384

hoodmane mentioned this pull request Mar 5, 2022

chore: matplotlib 3.5.1, yt 4 #2234

Merged

hoodmane added 4 commits March 5, 2022 00:03

Merge branch 'main' into better-cross-build

6c4f45a

Revert pyodide build tests changes

6d5fa97

Install benchmark requirements

d8d0a19

Merge branch 'main' into better-cross-build

bd0a549

ryanking13 reviewed Mar 8, 2022

View reviewed changes

pyodide-build/pyodide_build/buildpkg.py Show resolved Hide resolved

hoodmane mentioned this pull request Mar 8, 2022

No replay #2256

Merged

hoodmane added 3 commits March 7, 2022 21:56

Merge branch 'main' into better-cross-build

f570a53

Remove yt post script

d331ba8

xfail yt import test on node

27b6ba7

ryanking13 approved these changes Mar 10, 2022

View reviewed changes

packages/scipy/meta.yaml Outdated Show resolved Hide resolved

hoodmane and others added 3 commits March 9, 2022 17:23

Update packages/scipy/meta.yaml

e4403f2

Co-authored-by: Gyeongjae Choi <[email protected]>

Remove numpy skip-build patch

df1a41e

Fix word splitting on run_docker script (pyodide#2260)

d65552d

Merge branch 'main' into better-cross-build

e53b29c

hoodmane merged commit c01ab54 into pyodide:main Mar 10, 2022

hoodmane deleted the better-cross-build branch March 10, 2022 04:34

rth reviewed Mar 18, 2022

View reviewed changes

rgommers mentioned this pull request Aug 31, 2023

WIP: CI: switch Emscripten job to Meson numpy/numpy#24603

Closed

		WHEEL.write_text("\n".join(result_lines))


		def update_wheel_platform(build_dir: Path, wheel_dir: Path):

		del bash_runner.env["_PYTHON_HOST_PLATFORM"]


		def set_host_platform(wheel_dir: Path, platform: str):

Uh oh!

Conversation

hoodmane commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henryiii commented Mar 3, 2022

Uh oh!

hoodmane commented Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ryanking13 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hoodmane commented Mar 10, 2022

Uh oh!

hoodmane commented Mar 10, 2022

Uh oh!

rth left a comment

Choose a reason for hiding this comment

Uh oh!

rth Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

hoodmane Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

rth Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

hoodmane Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

rth Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

hoodmane Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hoodmane commented Mar 2, 2022 •

edited

Loading

hoodmane commented Mar 3, 2022 •

edited

Loading