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

Add support for cross #134

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 47 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ jobs:
continue
fi

tox -c $example_dir -e py
tox -c $example_dir -e py -vvvv
done

- name: Test macOS universal2
Expand Down Expand Up @@ -137,7 +137,7 @@ jobs:
cd examples/rust_with_cffi/
python --version
pip install wheel
python setup.py bdist_wheel --py-limited-api=cp35
python setup.py bdist_wheel --py-limited-api=cp36
ls -la dist/

# Now we switch to a differnet Python version and ensure we can install
Expand All @@ -155,3 +155,48 @@ jobs:
pip install rust_with_cffi/dist/rust_with_cffi*.whl
python -c "from rust_with_cffi import rust; assert rust.rust_func() == 14"
python -c "from rust_with_cffi.cffi import lib; assert lib.cffi_func() == 15"

test-cross:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@master
- name: Setup python
uses: actions/setup-python@v2
with:
python-version: 3.8
- uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: stable
override: true
- name: Install cross
run: cargo install cross
- name: Build package
run: pip install -e .
- name: Build wheel using cross
shell: bash
env:
CARGO: cross
CARGO_BUILD_TARGET: aarch64-unknown-linux-gnu
PYO3_CROSS_LIB_DIR: /opt/python/cp38-cp38/lib
run: |
cd examples/namespace_package
docker build -t cross-pyo3:aarch64-unknown-linux-gnu .
python -m pip install wheel
python setup.py bdist_wheel --plat-name manylinux2014_aarch64
ls -la dist/
- uses: uraimo/[email protected]
name: Install built wheel
with:
arch: aarch64
distro: ubuntu20.04
dockerRunArgs: |
--volume "${PWD}/examples/namespace_package:/io"
install: |
apt-get update
apt-get install -y --no-install-recommends python3 python3-pip
pip3 install -U pip
run: |
pip3 install namespace_package --no-index --find-links /io/dist/ --force-reinstall
python3 -c "from namespace_package import rust; assert rust.rust_func() == 14"
python3 -c "from namespace_package import python; assert python.python_func() == 15"
9 changes: 9 additions & 0 deletions examples/namespace_package/Cross.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
[target.aarch64-unknown-linux-gnu]
image = "cross-pyo3:aarch64-unknown-linux-gnu"

[build.env]
passthrough = [
"RUST_BACKTRACE",
"RUST_LOG",
"PYO3_CROSS_LIB_DIR",
]
13 changes: 13 additions & 0 deletions examples/namespace_package/Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
FROM quay.io/pypa/manylinux2014_aarch64 AS manylinux

FROM rustembedded/cross:aarch64-unknown-linux-gnu

RUN apt-get update && \
apt-get install -y software-properties-common && \
add-apt-repository ppa:deadsnakes/ppa && \
apt-get update && \
apt-get install -y python3.8 && \
update-alternatives --install /usr/bin/python3 python3 /usr/bin/python3.8 1

COPY --from=manylinux /opt/_internal /opt/_internal
COPY --from=manylinux /opt/python /opt/python
40 changes: 37 additions & 3 deletions setuptools_rust/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import platform
import shutil
import sys
import sysconfig
import subprocess
from distutils.errors import (
CompileError,
Expand Down Expand Up @@ -50,6 +51,7 @@ def initialize_options(self):
self.build_temp = None
self.plat_name = None
self.target = os.getenv("CARGO_BUILD_TARGET")
self.cargo = os.getenv("CARGO", "cargo")

def finalize_options(self):
super().finalize_options()
Expand Down Expand Up @@ -153,7 +155,7 @@ def build_extension(self, ext: RustExtension, target_triple=None):

if executable:
args = (
["cargo", "build", "--manifest-path", ext.path]
[self.cargo, "build", "--manifest-path", ext.path]
+ feature_args
+ target_args
+ list(ext.args or [])
Expand All @@ -169,7 +171,7 @@ def build_extension(self, ext: RustExtension, target_triple=None):

else:
args = (
["cargo", "rustc", "--lib", "--manifest-path", ext.path]
[self.cargo, "rustc", "--lib", "--manifest-path", ext.path]
+ feature_args
+ target_args
+ list(ext.args or [])
Expand Down Expand Up @@ -349,10 +351,42 @@ def get_dylib_ext_path(self, ext, target_fname):
assert modpath not in build_ext.ext_map
build_ext.ext_map[modpath] = ext
try:
return build_ext.get_ext_fullpath(target_fname)
ext_path = build_ext.get_ext_fullpath(target_fname)
finally:
del build_ext.ext_map[modpath]

if '.abi3.' in ext_path:
Copy link
Member

Choose a reason for hiding this comment

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

The logic from here down is quite complex. Can we split it into a few smaller functions, to help with cognitive load? e.g. is_linux_cross_compile() , strip_platform_and_extension()? That might be enough.

return ext_path
# Examples: linux_x86_64, linux_i686, manylinux2014_aarch64, manylinux_2_24_armv7l
plat_name = self.plat_name.lower().replace('-', '_').replace('.', '_')
if not plat_name.startswith(('linux', 'manylinux')):
return ext_path

arch_parts = []
arch_found = False
for item in plat_name.split('_'):
if item.startswith(('linux', 'manylinux')):
continue
if item.isdigit() and not arch_found:
# manylinux_2_24_armv7l arch should be armv7l
continue
arch_found = True
arch_parts.append(item)
target_arch = '_'.join(arch_parts)
host_platform = sysconfig.get_platform()
host_arch = host_platform.rsplit('-', 1)[1]
# Remove incorrect platform tag if we are cross compiling
if target_arch and host_arch != target_arch:
Comment on lines +365 to +379
Copy link
Member

Choose a reason for hiding this comment

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

Yikes. Do we also need to check abi here? e.g. windows-gnu versus windows-msvc?

Is it enough to check if plat_name doesn't match sysconfig.get_platform()?

Maybe we should remove the platform tag always, rather than try to do some clever detection? Depends if there's a reason to keep the platform tag around.

Copy link
Member

Choose a reason for hiding this comment

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

Or alternatively, looking at discussion in #138 it sounds like Python also gets this wrong? Maybe we should not fix the platform tag, and instead just leave it to users. If that's consistent with what C does, it seems most reliable (even if it's annoying).

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should remove the platform tag always, rather than try to do some clever detection?

I don't know, but I did encounter an error once with PyPy with no platform tag, see https://github.com/PyO3/setuptools-rust/runs/2208718989?check_suite_focus=true

Copy link
Member Author

Choose a reason for hiding this comment

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

Or alternatively, looking at discussion in #138 it sounds like Python also gets this wrong?

Yes, it's terrible.

Maybe we should not fix the platform tag, and instead just leave it to users. If that's consistent with what C does, it seems most reliable (even if it's annoying).

It would make supporting cross kinda useless IMO.

ext_dir = os.path.dirname(ext_path)
ext_name = os.path.basename(ext_path)
# rust.cpython-38-x86_64-linux-gnu.so to (rust.cpython-38-x86_64-linux-gnu, .so)
ext_name, ext_ext = os.path.splitext(ext_name)
# rust.cpython-38-x86_64-linux-gnu to (rust, .cpython-38-x86_64-linux-gnu)
ext_name, _plat_tag = os.path.splitext(ext_name)
Comment on lines +382 to +385
Copy link
Member

Choose a reason for hiding this comment

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

We do a similar pattern about line ~310, maybe can refactor to share a common util function?

Copy link
Member

Choose a reason for hiding this comment

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

Still would like to see this, maybe call strip_platform_and_extension() ?

# rust.so, removed platform tag
ext_path = os.path.join(ext_dir, ext_name + ext_ext)
return ext_path

@staticmethod
def create_universal2_binary(output_path, input_paths):
# Try lipo first
Expand Down
4 changes: 3 additions & 1 deletion setuptools_rust/setuptools_ext.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,11 @@ def run(self):
log.info("running build_rust")
build_rust = self.get_finalized_command("build_rust")
build_rust.inplace = self.inplace
build_rust.plat_name = self.plat_name
build_rust.target = self.target
build_rust.verbose = self.verbose
options = self.distribution.get_cmdline_options().get('bdist_wheel', {})
plat_name = options.get('plat-name') or self.plat_name
build_rust.plat_name = plat_name
build_rust.run()

build_ext_base_class.run(self)
Expand Down