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

Add support for cross #134

wants to merge 9 commits into from

Conversation

messense
Copy link
Member

Closes #10

@messense
Copy link
Member Author

creating build/bdist.linux-x86_64
creating build/bdist.linux-x86_64/wheel
creating build/bdist.linux-x86_64/wheel/namespace_package
creating build/bdist.linux-x86_64/wheel/namespace_package/python
copying build/lib/namespace_package/python/__init__.py -> build/bdist.linux-x86_64/wheel/namespace_package/python
copying build/lib/namespace_package/rust.cpython-38-x86_64-linux-gnu.so -> build/bdist.linux-x86_64/wheel/namespace_package
running install_egg_info
running egg_info
creating namespace_package.egg-info
writing namespace_package.egg-info/PKG-INFO
writing dependency_links to namespace_package.egg-info/dependency_links.txt
writing top-level names to namespace_package.egg-info/top_level.txt
writing manifest file 'namespace_package.egg-info/SOURCES.txt'
reading manifest file 'namespace_package.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
writing manifest file 'namespace_package.egg-info/SOURCES.txt'
Copying namespace_package.egg-info to build/bdist.linux-x86_64/wheel/namespace_package-0.1.0-py3.8.egg-info
running install_scripts
creating build/bdist.linux-x86_64/wheel/namespace_package-0.1.0.dist-info/WHEEL
creating 'dist/namespace_package-0.1.0-cp38-cp38-manylinux2014_aarch64.whl' and adding 'build/bdist.linux-x86_64/wheel' to it
adding 'namespace_package/rust.cpython-38-x86_64-linux-gnu.so'

Looks like #138 (comment)

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this, and also the great example / test.

I've had to do a lot of reading to get my head around all of this. I think I understand now, and I'll summarise below. Please correct any of this which is wrong:

  1. cross build will run the build command inside a dockerfile which contains rustc configured to cross-compile.
    • The dockerfile is still running the host architecture.
  2. In the example, we put a python installation inside the dockerfile so we can point PYO3_CROSS_LIB_DIR at it.
    • I think here we get lucky: we're targeting ARM but we're pointing PyO3 to a python installation on the host architecture.
  3. We have to remove the platform tag, because Python isn't aware we're cross compiling and so gives us the wrong extension filename.
  4. We use a github action which emulates ARM architecture to test the built wheel works.

So I have a concern about (2) that we're lucky we're getting a working build; PyO3 could easily get some wrong information from the PYO3_CROSS_LIB_DIR we provide it - it's for the host architecture, not the target architecture.

Maybe it's better for the example to cross-compile for a --py-limited-api where we don't need a cross lib dir? We might not need a custom Dockerfile at all in that case.

Alternatively I wonder if we can combine it with #139 to somehow set up crossenv inside the Dockerfile?

Comment on lines +382 to +385
# 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)
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() ?

Comment on lines +365 to +379
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:
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.

@messense
Copy link
Member Author

  1. In the example, we put a python installation inside the dockerfile so we can point PYO3_CROSS_LIB_DIR at it.
    I think here we get lucky: we're targeting ARM but we're pointing PyO3 to a python installation on the host architecture.

We didn't get lucky. The PYO3_CROSS_LIB_DIR is passed into Docker container where it is point to an aarch64 Python build from quay.io/pypa/manylinux2014_aarch64 docker image which is really aarch64 .

@messense
Copy link
Member Author

Maybe it's better for the example to cross-compile for a --py-limited-api where we don't need a cross lib dir? We might not need a custom Dockerfile at all in that case.

We can certainly do that. But I think it's useful to have an example without abi3.

Alternatively I wonder if we can combine it with #139 to somehow set up crossenv inside the Dockerfile?

crossenv currently has an issue with Rust built scripts which is partially addressed in benfogle/crossenv#66

@messense
Copy link
Member Author

BTW, using crossenv along with cross isn't really useful, we would also need a copy of aarch64 Python outside of Docker container to create the cross compilation virtualenv because we invoke python setup.py bdist_wheel outside of Docker container.

@messense
Copy link
Member Author

Note that I'm not keen on pursue this as cross official Docker images are not manylinux complaint.

@davidhewitt
Copy link
Member

aarch64 Python build from quay.io/pypa/manylinux2014_aarch64 docker image

Ah, yes I missed that the manylinux image chosen was for arm 👍

BTW, using crossenv along with cross isn't really useful, we would also need a copy of aarch64 Python outside of Docker container to create the cross compilation virtualenv because we invoke python setup.py bdist_wheel outside of Docker container.

Yeah that makes sense, thanks.

Note that I'm not keen on pursue this as cross official Docker images are not manylinux complaint.

You mean that you think we should close this PR instead of merging? I wonder how hard it would be for us to maintain a set of Docker images for cross. But maybe it's easier for us to instead work on buildroot + crossenv support?

@messense
Copy link
Member Author

messense commented Mar 29, 2021

You mean that you think we should close this PR instead of merging?

I'm fine with closing this PR. But it could be useful to build statically linked musl-libc executables which is manylinux compliant.

cc @wdv4758h @casellimarco

I wonder how hard it would be for us to maintain a set of Docker images for cross.

I'm not sure, cross relies on a lot of things in its Docker container. https://github.com/rust-embedded/cross/tree/master/docker

But maybe it's easier for us to instead work on buildroot + crossenv support?

I think so.

@wdv4758h
Copy link

I just see someone tagged me, didn't read through whole threads.

Good to see someone is still pushing this. I think Python's extension cross compile story is bad for quite long time ...
I don't do extension cross compile recently, but I can see some tricks I used couple years ago like removing wrong platform tag (was doing cross compile for extension in C).

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Sorry I delayed this for so long.

@messense has your opinion changed? I see you now maintain https://github.com/messense/manylinux-cross, so perhaps with good documentation we can help users use cross correctly?

If so, let's tidy this up and merge it :)

Comment on lines +382 to +385
# 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)
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() ?

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.

@messense
Copy link
Member Author

It's certainly useful for bin bindings, feel free to take over and get it to finish line, thanks!

@davidhewitt davidhewitt mentioned this pull request Sep 22, 2021
@morenol
Copy link

morenol commented Sep 22, 2021

I just tried this branch and it works for me to cross-compile a python package for a raspberry pi. Is there something that I can do to help this land to the main branch?

@messense
Copy link
Member Author

I just tried this branch and it works for me to cross-compile a python package for a raspberry pi. Is there something that I can do to help this land to the main branch?

A rebased version of this PR maybe?

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

Successfully merging this pull request may close these issues.

Support "cross"
4 participants