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

feat(bindings/python): build and publish aarch64 and armv7l wheels #3325

Merged
merged 2 commits into from
Oct 17, 2023

Conversation

messense
Copy link
Member

@messense messense commented Oct 17, 2023

@messense messense marked this pull request as ready for review October 17, 2023 15:06
@Xuanwo
Copy link
Member

Xuanwo commented Oct 17, 2023

PR failed for #3326, I'm working on this.

@Xuanwo
Copy link
Member

Xuanwo commented Oct 17, 2023

Test run: https://github.com/apache/incubator-opendal/actions/runs/6549011611?pr=3325

Closes #3311 cc @Zheaoli

Seems we are still building x86_64:

https://github.com/apache/incubator-opendal/actions/runs/6549011611/job/17784998166?pr=3325

📦 Built wheel for CPython 3.9 to dist/opendal-0.41.0-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
   Compiling pyo3-build-config v0.19.2
   Compiling pyo3-ffi v0.19.2
   Compiling pyo3 v0.19.2
   Compiling pyo3-asyncio v0.19.0
   Compiling opendal-python v0.41.0 (/home/runner/work/incubator-opendal/incubator-opendal/bindings/python)
    Finished release [optimized] target(s) in 27.79s

Maybe we should pass architecture to PyO3/maturin-action?

@messense
Copy link
Member Author

Oops, forgot to do that.

@messense
Copy link
Member Author

warning: In file included from /root/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.17.4/pregenerated/aesv8-armx-linux64.S:4:0:
warning: include/ring-core/asm_base.h:73:2: error: #error "ARM assembler must define __ARM_ARCH"
warning:  #error "ARM assembler must define __ARM_ARCH"
warning:   ^
warning: In file included from /root/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.17.4/pregenerated/aesv8-armx-linux64.S:7:0:
warning: include/ring-core/arm_arch.h:82:2: error: #error "ARM assembler must define __ARM_ARCH"
warning:  #error "ARM assembler must define __ARM_ARCH"
warning:   ^

error: failed to run custom build command for `ring v0.17.4`

Ok, why are we depending on ring 0.17? rustls doesn't have a release with ring 0.17 support yet I think?

@messense
Copy link
Member Author

Hmm, we have duplicate ring dependencies:

$ cargo tree -i ring
error: There are multiple `ring` packages in your project, and the specification `ring` is ambiguous.
Please re-run this command with `-p <spec>` where `<spec>` is one of the following:
  [email protected]
  [email protected]

$ cargo tree -i [email protected]
ring v0.17.4
└── jsonwebtoken v9.0.0
    └── reqsign v0.14.2
        └── opendal v0.41.0 (/Users/messense/Projects/opendal/core)

@Xuanwo
Copy link
Member

Xuanwo commented Oct 17, 2023

Hmm, we have duplicate ring dependencies:

Yes, we're waiting for other dependencies such as reqwest to catch up.

@Xuanwo
Copy link
Member

Xuanwo commented Oct 17, 2023

Ok, why are we depending on ring 0.17? rustls doesn't have a release with ring 0.17 support yet I think?

Yes, but how does this impact rustls? We have other dependencies that rely directly on ring. I thought rustls will still use ring 0.16.

@messense
Copy link
Member Author

messense commented Oct 17, 2023

It's not ideal to have duplication for such crucial crypto dependency, we should wait for the ecosystem to upgrade before doing our upgrade.

Besides, the manylinux GCC cross compilers seems to have some issues with ring 0.17 namely __ARM_ARCH from Arm C Language Extensions (ACLE). I don't know why boringssl/ring use this while openssl uses some other feature detection macros for the same purpose. (ring 0.16 was using the same macros as openssl so it doesn't suffer from this issue.)

@Xuanwo
Copy link
Member

Xuanwo commented Oct 17, 2023

Thanks for the explanation!

@messense
Copy link
Member Author

messense commented Oct 17, 2023

Besides, the manylinux GCC cross compilers seems to have some issues with ring 0.17 namely __ARM_ARCH from Arm C Language Extensions (ACLE).

Probably because GCC 4.8.5 is too old to fully support ACLE. 😂

https://boringssl.googlesource.com/boringssl/+/HEAD/BUILDING.md#build-prerequisites

Compilers for C11 and C++14, or later, are required. On Windows, MSVC from Visual Studio 2019 or later with Windows 10 SDK 2104 or later are supported, but using the latest versions is recommended. Recent versions of GCC (6.1+) and Clang should work on non-Windows platforms, and maybe on Windows too.

@messense
Copy link
Member Author

Seems we are still building x86_64:

Fixed, see https://github.com/apache/incubator-opendal/actions/runs/6549415727?pr=3325

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks!

@Xuanwo Xuanwo merged commit ec47377 into apache:main Oct 17, 2023
@messense messense deleted the python-arm-wheels branch October 18, 2023 02:22
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.

Release Python wheel for different Python version and architectures
2 participants