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.

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

Wheels for musl Linux #6245

Merged
merged 16 commits into from
Oct 2, 2021
Merged

Wheels for musl Linux #6245

merged 16 commits into from
Oct 2, 2021

Conversation

bwoodsend
Copy link
Member

@bwoodsend bwoodsend commented Sep 25, 2021

This is the setting up for building PyPI friendly wheels for musl.

  • Put musl bootloaders in their own PyInstaller/bootloader/Linux-64bit-intel-musl (assuming x86_64) folder.
  • Define python setup.py wheel_linux_64bit_intel_musl and python setup wheel_linux_64bit_arm_musl.
  • Add a docker file for testing Alpine.

Unfortunately, the build system for producing wheels is getting more and more complicated. Because dockcross don't (yet) support the musllinux family of build images, we have to use the official PyPA non-cross variants which require docker buildx + qemu to virtualise aarch64 inside x86_64. I'll be quite surprised if WSL2 can do this so we may need to shunt this to CI or build locally on a real Linux machine.

In any case, it adds the following two commands to be ran before python setup.py bdist_wheels:

docker run -v "$(pwd):/io" -it $(docker build --build-arg BASE=quay.io/pypa/musllinux_1_1_x86_64 -q ./bootloader)
docker run -v "$(pwd):/io" -it $(docker build --build-arg BASE=quay.io/pypa/musllinux_1_1_aarch64 -q ./bootloader)

I'd also like everyone's thoughts on whether we should add the testing on alpine dockerfile to CI? Without installing everything from tests/requirements-libraries.txt it only takes about 20 minutes to run the entire test suite without parallelising and about 10 with so I'd vote yes. There are a few failures which all boil down to the same cause - namely that ldconfig -p doesn't work with musl's variant of ldconfig so ctypes.util.find_library() yields nothing (although it has a fallback to using gcc to resolve libraries if that's installed) and PyInstaller.depend.utils.load_ldconfig_cache() also yields an empty mapping. So we'd have to mop those up before making musl testing routine.

@bwoodsend bwoodsend force-pushed the many-musl branch 2 times, most recently from 7276af8 to 610bc62 Compare September 25, 2021 16:37
@bwoodsend
Copy link
Member Author

Here's a run of the test suite on Alpine as it is: alpine.log

bootloader/Dockerfile Outdated Show resolved Hide resolved
@rokm
Copy link
Member

rokm commented Sep 26, 2021

I'd also like everyone's thoughts on whether we should add the testing on alpine dockerfile to CI?

I take it this would be a reduced workflow (compared to the full-blown CI) that avoids installing everything from requirements-libraries.txt (at least until musl wheels become available for non-pure-python packages)? So unit tests plus basic functionality?

@bwoodsend
Copy link
Member Author

Definitely we would have to skip requirements-libraries.txt. Otherwise it will take days to compile the likes of scipy, matplotlib and Qt from source - as it is, it already takes a few minutes just to compile lxml from source. As the alpine dockerfile stands at the moment it installs requirements-tools.txt only so it should be testing everything par our hooks for non standard library packages.

@bwoodsend
Copy link
Member Author

Error relocating /usr/local/lib/python3.9/lib-dynload/math.cpython-39-x86_64-linux-gnu.so: PyLong_AsLongAndOverflow: symbol not found
Error relocating /usr/local/lib/python3.9/lib-dynload/math.cpython-39-x86_64-linux-gnu.so: _PyObject_LookupSpecial: symbol not found
Error relocating /usr/local/lib/python3.9/lib-dynload/math.cpython-39-x86_64-linux-gnu.so: PyNumber_Subtract: symbol not found
Error relocating /usr/local/lib/python3.9/lib-dynload/math.cpython-39-x86_64-linux-gnu.so: PyMem_Malloc: symbol not found
...
Error relocating /usr/local/lib/python3.9/lib-dynload/math.cpython-39-x86_64-linux-gnu.so: _PyLong_Sign: symbol not found
Error relocating /usr/local/lib/python3.9/lib-dynload/math.cpython-39-x86_64-linux-gnu.so: PyNumber_Absolute: symbol not found
Error relocating /usr/local/lib/python3.9/lib-dynload/_tkinter.cpython-39-x86_64-linux-gnu.so: _Py_TrueStruct: symbol not found
Error relocating /usr/local/lib/python3.9/lib-dynload/_tkinter.cpython-39-x86_64-linux-gnu.so: PyOS_InputHook: symbol not found
Error relocating /usr/local/lib/python3.9/lib-dynload/_tkinter.cpython-39-x86_64-linux-gnu.so: PyObject_GenericGetAttr: symbol not found

@rokm Can you remember what these mean? I'm sure I've seen them before but I can't remember where.

@rokm
Copy link
Member

rokm commented Sep 26, 2021

Error relocating /usr/local/lib/python3.9/lib-dynload/math.cpython-39-x86_64-linux-gnu.so: PyNumber_Absolute: symbol not found

These are harmless.

They are caused by ldd not being able to resolve the symbols (e.g., try runningldd /usr/lib/python3.9/lib-dynload/math.cpython-39-x86_64-linux-musl.so in the alpine container), because the extension is not actually linked against the python shared library (it doesn't have to be, because by the time it is loaded, the python shared library is already loaded in the process, and thus the symbols are available...).

I've checked and on my Fedora system, the extensions are also not linked against python shared library - so perhaps the ldd on Alpine is more verbose and complains about missing symbols, whereas the ldd on glibc-based systems does not.

@rokm
Copy link
Member

rokm commented Sep 26, 2021

We might want to capture the stderr of the ldd subprocess, and either display it only on error or display a filtered version (with entries matching Error relocating ...: symbol not found removed) to avoid spamming the build log with those messages.

@bwoodsend
Copy link
Member Author

Ahh it's ldd's output. I was git grep-ing around PyInstaller's code base looking for Error relocating patterns. I think captured and filtered stderr sounds sensible.

@bwoodsend bwoodsend force-pushed the many-musl branch 2 times, most recently from 28feffe to fb677ac Compare September 28, 2021 20:29
@bwoodsend
Copy link
Member Author

bwoodsend commented Sep 28, 2021

This should make life painfull interesting. The macOS-latest image has finally switched to Big Sur.

@rokm
Copy link
Member

rokm commented Sep 28, 2021

This should make life painfull interesting. The macOS-latest image has finally switched to Big Sur.

Hmm, let's force the CI to macos-10.15 for the time being, then. I'll take a look at bringing #5488 up to date, and then we can switch back (or even run tests on both)...

@bwoodsend bwoodsend force-pushed the many-musl branch 4 times, most recently from f968737 to 02b0ce8 Compare September 29, 2021 10:42
@bwoodsend
Copy link
Member Author

bwoodsend commented Sep 29, 2021

Provided the rest of CI doesn't have any hidden surprises in store, that's this ready for review. CC @Legorooj so that you're aware of what's going on.

To summarise the important bits:

  • c00332f 738c6f2 8c0d861: Allow us to build musl wheels.
  • 7325d71 143d052 e661b2b: A few changes to make its test suite happier.
  • 8582b67: Skip one test which is inherently doomed on musl.
  • 03f73b9 40e81dc: Skip two tests I just couldn't get to work. I think it's safe to assume that the first is not PyInstaller's fault. The second I'm not sure what's wrong with it but it's only a logger.debug() getting lost so I'm not particularly sad to write it off.
  • 2b3cfca e8a05bc: Test Alpine locally and on CI.
  • 054fa4f: Unless anyone objects, I'm moving the building of musl bootloaders and glibc + aarch64 bootloaders to CI for convenience. I think, given that they're all being built in containers, the security concerns with shipping executables built on CI are effectively moot. Here is a build run of the wheel building workflow.

@bwoodsend bwoodsend marked this pull request as ready for review September 29, 2021 12:17
Copy link
Member

@Legorooj Legorooj left a comment

Choose a reason for hiding this comment

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

Looking good! However I'm not comfortable autocompiling the bootloaders in the wheel builder pipeline like that. No objections to building them in CI, just not like that.

Can we drop it for this PR and I'll work up something different afterwards, that works for building all the bootloaders?

@bwoodsend
Copy link
Member Author

Can we drop it for this PR and I'll work up something different afterwards, that works for building all the bootloaders?

My reason for building only those bootloaders here was because those are the ones we don't check into the repository so building them here means that we don't have to juggle those bootloaders between workflows. But sure, I'll drop it for now if you have other plans.

@rokm
Copy link
Member

rokm commented Oct 2, 2021

Regarding the mystery of 40e81dc: does it help if you specify the logger we are supposed to get the message from? I.e., change the

with caplog.at_level(logging.DEBUG):

into

with caplog.at_level(logging.DEBUG, logger='PyInstaller.utils.hooks'):

PyInstaller/__init__.py Outdated Show resolved Hide resolved
news/6245.bugfix.rst Outdated Show resolved Hide resolved
tests/unit/test_bindepend.py Outdated Show resolved Hide resolved
This will allow us to keep musl and glibc Linux bootloaders separate in
the repository without them clobbering each other.
@bwoodsend
Copy link
Member Author

Regarding the mystery of 40e81dc: does it help if you specify the logger we are supposed to get the message from? I.e., change the

with caplog.at_level(logging.DEBUG):

into

with caplog.at_level(logging.DEBUG, logger='PyInstaller.utils.hooks'):

Yes it does. And it wasn't specific to musl after all - I can reproduce the error on normal Linux. So mystery solved...

These wheels can be uploaded to PyPI and ran on Alpine or OpenWRT.
Musl does not use ldconfig cache at all. The ldconfig executable, strictly
speaking, is not supposed to exist on a traditional musl system but a dummy
executable which complains if it receives any arguments may be put in its place
for compatibility. Running this dummy should be avoided because it writes a
visible error message to stderr.

Update the corresponding test to reflect that LDCONFIG_CACHE will always be
empty on musl.
Namely, findLibrary("libc") would pick any library matching "libc*" which
may be libc.so or it may be some other library such as libcrypto.so.
... due to the fact that said function relies on ldconfig which on musl either
doesn't exist or does nothing. Note that find_library() does have a fallback to
using gcc to find libraries if gcc is installed but gcc should not be a test
time dependency.
Tcl and musl don't appear to get on well. Both `python -m tkinter` and `git gui`
(which uses tcl) raise segfaults. They both work fine in ubuntu containers and
gtk applications work fine in alpine containers so the issue is specific to musl
+ tcl.
tests/unit/test_hookutils.py Outdated Show resolved Hide resolved
The failure can be reproduced only by running at least one functional test
which invokes PyInstaller main() before the offending logger test:

  pytest tests/functional/test_basic.py::test_absolute_python_path[onedir] tests/unit/test_hookutils.py
@bwoodsend
Copy link
Member Author

I take it that we're all agreed that there's not much point testing musl on all Python versions rather than just 3.9? I think the chances of finding a libc implementation and Python version specific quirk that derails PyInstaller is too low to warrant clogging CI up with more jobs.

@rokm
Copy link
Member

rokm commented Oct 2, 2021

I take it that we're all agreed that there's not much point testing musl on all Python versions rather than just 3.9? I think the chances of finding a libc implementation and Python version specific quirk that derails PyInstaller is too low to warrant clogging CI up with more jobs.

Yeah, I agree.

@bwoodsend bwoodsend merged commit 1505edb into pyinstaller:develop Oct 2, 2021
@bwoodsend bwoodsend added the backported Backported to v4 branch label Oct 2, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backported Backported to v4 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants