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

musslinux: Always (un)pack structs as little endian #473

Closed
wants to merge 1 commit into from

Conversation

hroncok
Copy link
Contributor

@hroncok hroncok commented Oct 26, 2021

Fixes problems on big endian architectures --
native order is used by default otherwise.

Fixes #472

Fixes problems on big endian architectures --
native order is used by default otherwise.

Fixes pypa#472
@hroncok
Copy link
Contributor Author

hroncok commented Oct 26, 2021

Disclaimer: I have no idea if this works if a musl binary for big endian system exists (ever). But it fixes the current test failures.

@uranusjr
Copy link
Member

I have no idea what a musl binary should look like on BE either, but judging from how _manylinux implements it I think they should be ordered as BE. The manylinux tests will probably fail if that's true, although I didn't check.

If my assumption is correct (no guarantees), the current implementation is probably correct (without this PR), and it's the test binaries we're using that need fixing.

@hroncok hroncok marked this pull request as draft October 26, 2021 10:09
@hroncok
Copy link
Contributor Author

hroncok commented Oct 26, 2021

Converted to draft then.

@glaubitz
Copy link

Disclaimer: I have no idea if this works if a musl binary for big endian system exists (ever). But it fixes the current test failures.

I tested this change on Debian unstable/s390x and it introduces a number of regressions:

FAILED tests/test_tags.py::TestManylinuxPlatform::test_linux_platforms_32_64bit_on_64bit_os[linux-x86_64-False-linux_x86_64] - OSErro...
FAILED tests/test_tags.py::TestManylinuxPlatform::test_linux_platforms_32_64bit_on_64bit_os[linux-x86_64-True-linux_i686] - OSError: ...
FAILED tests/test_tags.py::TestManylinuxPlatform::test_linux_platforms_32_64bit_on_64bit_os[linux-aarch64-False-linux_aarch64] - OSEr...
FAILED tests/test_tags.py::TestManylinuxPlatform::test_linux_platforms_32_64bit_on_64bit_os[linux-aarch64-True-linux_armv7l] - OSErro...
FAILED tests/test_tags.py::TestManylinuxPlatform::test_linux_platforms_manylinux_unsupported - OSError: [Errno 22] Invalid argument
FAILED tests/test_tags.py::TestManylinuxPlatform::test_linux_platforms_manylinux1 - OSError: [Errno 22] Invalid argument
FAILED tests/test_tags.py::TestManylinuxPlatform::test_linux_platforms_manylinux2010 - OSError: [Errno 22] Invalid argument
FAILED tests/test_tags.py::TestManylinuxPlatform::test_linux_platforms_manylinux2014 - OSError: [Errno 22] Invalid argument
FAILED tests/test_tags.py::TestManylinuxPlatform::test_linux_platforms_manylinux2014_armv6l - OSError: [Errno 22] Invalid argument
FAILED tests/test_tags.py::TestSysTags::test_linux_cpython - OSError: [Errno 22] Invalid argument
FAILED tests/test_tags.py::TestSysTags::test_linux_platforms_manylinux2014_armv6l - OSError: [Errno 22] Invalid argument
FAILED tests/test_tags.py::TestSysTags::test_skip_manylinux_2014 - OSError: [Errno 22] Invalid argument
FAILED tests/test_tags.py::TestSysTags::test_linux_use_manylinux_compatible[x86_64-2-20-False] - OSError: [Errno 22] Invalid argument
FAILED tests/test_tags.py::TestSysTags::test_linux_use_manylinux_compatible[s390x-2-22-True] - OSError: [Errno 22] Invalid argument
FAILED tests/test_tags.py::TestSysTags::test_linux_use_manylinux_compatible_none - OSError: [Errno 22] Invalid argument
FAILED tests/test_tags.py::TestSysTags::test_pypy_first_none_any_tag - OSError: [Errno 22] Invalid argumen

@uranusjr
Copy link
Member

uranusjr commented Apr 19, 2022

@glaubitz If you have access to a Big Endian machine, could you compile the test binaries (see tests/musllinux/build.sh) and upload them somewhere so I can inspect how they are actually structured, and use them to fix the tests if possible? It’d be even more awesome if build.sh could be modified in a way that binaries can be updated in the future on a BE machine.

@hroncok
Copy link
Contributor Author

hroncok commented Apr 19, 2022

If you have access to a Big Endian machine...

I do. Will give it a try.

@hroncok
Copy link
Contributor Author

hroncok commented Apr 19, 2022

--- a/tests/musllinux/build.sh
+++ b/tests/musllinux/build.sh
@@ -18,23 +18,20 @@ build_one_in_ubuntu () {
 }
 
 build_one_in_alpine () {
-    $1 "multiarch/alpine:${2}-${ALPINE_VERSION}" \
+    $1 "s390x/alpine" \
         sh "/home/hello-world/musllinux/build.sh" musl "musl-${2}"
 }
 
 build_in_container () {
     local SOURCE="$(dirname $(dirname $(realpath ${BASH_SOURCE[0]})))"
-    DOCKER="docker run --rm -v ${SOURCE}:/home/hello-world"
+    DOCKER="podman run --security-opt label=disable --rm -v ${SOURCE}:/home/hello-world"
 
     if [[ $# -ne 0 ]]; then
         "build_one_in_${1}" "$DOCKER" "$2"
         return
     fi
 
-    build_one_in_alpine "$DOCKER" x86_64
-    build_one_in_alpine "$DOCKER" i386
-    build_one_in_alpine "$DOCKER" aarch64
-    build_one_in_ubuntu "$DOCKER" x86_64
+    build_one_in_alpine "$DOCKER" s390x
 }

musl-s390x.zip

@hroncok
Copy link
Contributor Author

hroncok commented Apr 19, 2022

FWIW I can run that script like that from an s390x machine as well as from an x86_64 machine and the result is identical.

@uranusjr
Copy link
Member

uranusjr commented Apr 20, 2022

Yup the executable is in BE. It seems that the part getting the interpreter string isn’t quite right, I’ll submit a patch later for it.

The problem is how we can structure the tests so they cover both endians. Do we need to?

@brettcannon
Copy link
Member

@hroncok did you want to handle the merge conflicts and take this out of draft?

@hroncok
Copy link
Contributor Author

hroncok commented Aug 19, 2022

Honestly, I don't know what this is about any more. I'll try to check it out, but there's always too many things to care about :(

@uranusjr
Copy link
Member

I think #538 covers this one (with a different approach but the same result)

@brettcannon
Copy link
Member

@hroncok I'm going to go ahead and close this then with the assumption that Tzu-Ping is right that it's already fixed. Thanks for trying to fix the issue!

@hroncok hroncok deleted the s390x branch August 23, 2022 18:58
@hroncok
Copy link
Contributor Author

hroncok commented Aug 23, 2022

Verified in #472 (comment)

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

Successfully merging this pull request may close these issues.

musllinux tests fail on s390x
4 participants