Skip to content

No replay#2256

Merged
hoodmane merged 68 commits intopyodide:mainfrom
hoodmane:no-replay
Mar 13, 2022
Merged

No replay#2256
hoodmane merged 68 commits intopyodide:mainfrom
hoodmane:no-replay

Conversation

@hoodmane
Copy link
Member

@hoodmane hoodmane commented Mar 7, 2022

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.

hoodmane added 30 commits March 1, 2022 18:25
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.
Rather than recording the first pass and then replaying it.
Build systems are allowed to implement arbitrary logic
@hoodmane hoodmane marked this pull request as ready for review March 7, 2022 23:38
@hoodmane hoodmane mentioned this pull request Mar 13, 2022
Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

Thanks for your effort in working on this @hoodmane! This change will definitely make understanding our build system much easier.

The overall changes LGTM, please update docs on our build system, and the changelog update that briefly describes changes in our build system would be great.

@hoodmane
Copy link
Member Author

Thanks for the review @ryanking13!

@hoodmane hoodmane merged commit f7b0f8c into pyodide:main Mar 13, 2022
Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @hoodmane, it looks very neat! Also thanks for the reviews @ryanking13 !

If there are no two stage builds anymore, wouldn't that mean that https://pyodide.org/en/latest/development/new-packages.html#partial-rebuilds need updating?

Also I don't really understand how building packages for host happens with this PR (e.g. numpy installed on host). We do have to run the build once for each arch don't we?
Maybe we could use this opportunity to document how cross-compilation works in broad terms in Pyodide, possibly by adding a page on "Build system" with a "Cross-compilation" subsection nested under https://pyodide.org/en/latest/development/new-packages.html (same as "The meta.yaml specification") or otherwise in the top level of "Development"?

[
"-Werror=implicit-function-declaration",
"-Werror=mismatched-parameter-types",
"-Werror=return-type",
Copy link
Member

Choose a reason for hiding this comment

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

Can we document this somewhere, under package building? Otherwise it's easy to forget that these are injected..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants