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

Enable crosscompiling aarch64 python wheels under dockcross manylinux docker image #8280

Merged
merged 2 commits into from
Mar 30, 2021

Conversation

jtattermusch
Copy link
Contributor

@jtattermusch jtattermusch commented Feb 10, 2021

Build crosscompiled aarch64 python wheels under dockcross manylinux2014 image.

Supersedes #8196 (some of the challenges I'm solving in this PR are already described there).

Key points:

  • manylinux2014 should be good enough for the aarch wheels (manylinux2014 seems to be the first manylinux version that has official aarch64 support).
  • a few workarounds are applied but overall the diff is pretty small and change changes integrate pretty well with the existing multibuild-based workflow for building python wheels.
  • the build time for an aarch64 wheel is comparable to the build of x86_64 wheel (which is expected, as this is doing crosscompilation).
  • the advantage of the setup in this PR is that all the linux wheels can be built on a single kokoro x64 machine and there are no changes needed to the release process and / or the way the wheels are being collected after being built (which seems quite important due to the complexity of the release process). Another obvious advantage is that no arm64 hardware is needed.

I smoke tested the resulting aarch64 wheel manually on a real arm64 machine and I also ran "auditwheel show" and no issues were reported - so I'm quite confident that they work just fine.

@jtattermusch
Copy link
Contributor Author

@haberman @janaknat please also take a look.

# when crosscompiling for aarch64, override the default multibuild's repair_wheelhouse logic
# since "auditwheel repair" doesn't work for crosscompiled wheels
function repair_wheelhouse {
echo "Skipping repair_wheelhouse since auditwheel requires build architecture to match wheel architecture."

Choose a reason for hiding this comment

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

build_wheel() can be modified to use a qemu-aarch64 wrapper with auditwheel:

# Based on multibuild build_wheel_cmd(). The only change is calling the repair_wheelhouse with qemu-aarch64.
function build_wheel_aarch64 {
    local cmd=${1:-pip_wheel_cmd}
    local repo_dir=${2:-$REPO_DIR}
    [ -z "$repo_dir" ] && echo "repo_dir not defined" && exit 1
    local wheelhouse=$(abspath ${WHEEL_SDIR:-wheelhouse})
    start_spinner
    if [ -n "$(is_function "pre_build")" ]; then pre_build; fi
    stop_spinner
    if [ -n "$BUILD_DEPENDS" ]; then
        pip install $(pip_opts) $BUILD_DEPENDS
    fi
    (cd $repo_dir && $cmd $wheelhouse)
    qemu-aarch64 repair_wheelhouse $wheelhouse
}

function build_wheel {
    if [ $ARCH == "arm64" ]; then
        build_wheel_aarch64 "bdist_wheel_cmd" $@
    else
        build_wheel_cmd "bdist_wheel_cmd" $@
    fi
}

This way you can keep the build and repair process within the container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting idea, but are you sure this is actually something that works? It seemed to me that you'd need to be in a aarch64 linux image (with arm-based python and binaries auditwheel uses, e.g. patchelf) for this to work.

@hanxiao
Copy link

hanxiao commented Feb 17, 2021

only one broken test left? way to go & looking forward to the merge!

@jtattermusch
Copy link
Contributor Author

@dlj-NaN @haberman friendly ping. I'd really like to hear feedback on whether this is an approach we could use (I believe so).

I'm aware that before publishing "official" version of the aarch64 wheels we'd need to have some aarch64 tests in place first (I have some ideas around how to set that up aarch64 tests under an emulator), but that is something that should be done in a separate PR. The wheel crosscompilation itself (as presented in this PR) seems quite reasonable to me. If this looks good, we could look into setting up emulated tests as the next step.

@jtattermusch
Copy link
Contributor Author

only one broken test left? way to go & looking forward to the merge!

The "Linux C++ Distcheck" failure is very likely unrelated to this PR.

Copy link
Contributor

@dlj-NaN dlj-NaN left a comment

Choose a reason for hiding this comment

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

One question before I merge this...

kokoro/release/python/linux/config.sh Show resolved Hide resolved
local plat_name_flag="--plat-name=$AUDITWHEEL_PLAT"

# override the value of EXT_SUFFIX to make sure the crosscompiled .so files in the wheel have the correct filename suffix
export PROTOCOL_BUFFERS_OVERRIDE_EXT_SUFFIX="$(python -c 'import sysconfig; print(sysconfig.get_config_var("EXT_SUFFIX").replace("-x86_64-linux-gnu.so", "-aarch64-linux-gnu.so"))')"
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be neat if you could do something like this:

$(qemu-aarch64 python -c 'import sysconfig; print(sysconfig.get_config_var("EXT_SUFFIX")')

I don't think that would currently work, since qemu won't necessarily have access to its own python... unless I missed something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, currently this won't work as there is no arm-specific installation of python in the docker image.
We can revisit things like that in the next iteration (along with figuring out how to run auditwheel checks e.g. under an emulator). I am going to continue looking into improvements like this while I'm experimenting with running protobuf tests under an emulator.

@jtattermusch
Copy link
Contributor Author

@dlj-NaN thanks for the review! Note that once we merge this, the python release builds will start generating the aarch64 wheels automatically (which I think according to your release process will end up uploading aarch64 wheels to pypi once the next release happens). If you want to avoid publishing the aarch64 wheels (until we have more tests in place), we need to explicitly remove them from the list of wheels that get published - let me know if you want me to look into that.

@jtattermusch
Copy link
Contributor Author

@dlj-NaN thanks for the review! Note that once we merge this, the python release builds will start generating the aarch64 wheels automatically (which I think according to your release process will end up uploading aarch64 wheels to pypi once the next release happens). If you want to avoid publishing the aarch64 wheels (until we have more tests in place), we need to explicitly remove them from the list of wheels that get published - let me know if you want me to look into that.

@dlj-NaN please let me know if I should look into removing the aarch64 wheels from the published set or if it's ok to leave as is (and merge this).

@dlj-NaN
Copy link
Contributor

dlj-NaN commented Mar 4, 2021

Yes, let's remove the wheels from the distribution until we can test them.

@jtattermusch
Copy link
Contributor Author

@dlj-NaN I just added a PR that provides a sufficient level of testing (at least IMHO) under aarch64 emulator: #8392. The auditwheel check passes, the wheels are installable and all the python unit tests (with cpp extension enabled) are passing.

@dlj-NaN
Copy link
Contributor

dlj-NaN commented Mar 23, 2021

@dlj-NaN I just added a PR that provides a sufficient level of testing (at least IMHO) under aarch64 emulator: #8392. The auditwheel check passes, the wheels are installable and all the python unit tests (with cpp extension enabled) are passing.

Until we have a chance to actually verify behavior and stability, it still seems prudent not to include release wheels yet.

@jtattermusch
Copy link
Contributor Author

@dlj-NaN I just added a PR that provides a sufficient level of testing (at least IMHO) under aarch64 emulator: #8392. The auditwheel check passes, the wheels are installable and all the python unit tests (with cpp extension enabled) are passing.

Until we have a chance to actually verify behavior and stability, it still seems prudent not to include release wheels yet.

Sounds fair. I have made a small change that puts the aarch64 manylinux artifacts into artifacts/unofficial directory, so that they are still being stored (for manual testing or unofficial releases), but they won't be uploaded to PyPI since the protobuf python release process only considers wheels that are directly under the artifacts/ directory.

With this change, the PR should be good to merge.

@hanxiao
Copy link

hanxiao commented Mar 28, 2021

so that they are still being stored (for manual testing or unofficial releases)

is there a link where one can fetch those unofficial releases/wheels?

@jtattermusch
Copy link
Contributor Author

jtattermusch commented Mar 28, 2021

so that they are still being stored (for manual testing or unofficial releases)

is there a link where one can fetch those unofficial releases/wheels?

They are accessible through the "Linux Python Release" job's artifacts. Unfortunately at this point, the results for those jobs are internal-only. Let's look into making the aarch64 wheels externally accessible as a follow up step.

@dlj-NaN dlj-NaN merged commit 0ebbd7d into protocolbuffers:master Mar 30, 2021
@hanxiao
Copy link

hanxiao commented Mar 30, 2021

Let's look into making the aarch64 wheels externally accessible as a follow up step.

👍

@iemejia
Copy link

iemejia commented Apr 7, 2021

Any ETA for the aarch64 wheels into PyPI ?

jtattermusch added a commit to jtattermusch/protobuf that referenced this pull request Jun 18, 2021
jtattermusch added a commit that referenced this pull request Jun 18, 2021
Reintroduce setup.py changes from #8280 erased by piper import #8617
jtattermusch added a commit to jtattermusch/protobuf that referenced this pull request Sep 10, 2021
haberman added a commit that referenced this pull request Sep 10, 2021
Reintroduce setup.py changes from #8280 erased by piper import #8902
jtattermusch added a commit to jtattermusch/protobuf that referenced this pull request Sep 15, 2021
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.

6 participants