ICRAR / crc32c

A python package implementing the crc32c algorithm in hardware and software
GNU Lesser General Public License v2.1
41 stars 25 forks source link

Fix cross-compiling #10

Closed eigendude closed 5 years ago

eigendude commented 5 years ago

Description

When packaging crc32c for OpenEmbedded, I hit a build error. Cross-compiling fails because setup.py assumes it's running on the build machine. However, setup.py treats the command line as an opaque string, so we can't pass build system parameters normally.

Fortunately, setuptools uses a file called setup.cfg to set parameters. I added support for machine configuration in a new build system section.

How has this been tested?

I created an OpenEmbedded recipe for crc32c:

DESCRIPTION = "A python package exposing the Intel SSE4.2 CRC32C instruction."
HOMEPAGE = "https://github.com/ICRAR/crc32c"
SECTION = "devel/python"
LICENSE = "LGPLv2"
LIC_FILES_CHKSUM = "file://setup.py;beginline=81;endline=81;md5=9e2e837c46174eef7a5db8b5c82dd34f"

SRC_URI[md5sum] = "9d799f4c836c003eb7fe23545c78e9f6"
SRC_URI[sha256sum] = "bdcd28f26b62838919480d465a0d166207a36c4f104102a0b6edf5b498544d36"

inherit pypi setuptools

Without the patch, I get this error:

| arm-sundstrom-linux-gnueabi-gcc: error: unrecognized command line option '-msse4.2'
| arm-sundstrom-linux-gnueabi-gcc: error: unrecognized command line option '-mpclmul'

Then I apply the patch, and append this to my recipe:

distutils_do_configure_prepend() {
    sed -i 's/#machine =.*/machine = ${MACHINE_ARCH}/' ${S}/setup.cfg
}

After the patch, crc32c can be used in the completed build.

rtobar commented 5 years ago

Hi,

Thanks for showing interest in the package, and for highlighting the problem of cross-compilation. In principle I think the idea is good, but I'm not convinced this is the best approach. First, it requires the setup.cfg file to be always present, but secondly it bypasses the normal distutils mechanisms to handle extra options. Given that our setup.py already provides its own build_ext command it would be easy to add an option there to specify the target platform, and have it default to the host platform.

I've pushed a commit in a new platform_option showing what I mean. If you agree I'd prefer to go in that direction, which integrates better with the rest of distutils, it doesn't require new modules to be added to our codebase, works correctly in the absence of a setup.cfg file, and also allows users to specify the option in the command line (i.e., python setup.py build_ext --platform=arm). In your recipe you can then change the sed command to something like echo -e "[build_ext]\nplatform = ${MACHINE_ARCH}" > setup.cfg.

eigendude commented 5 years ago

I was unaware of how build_ext fit into the picture. This is much nicer! I'll try your patch now.

eigendude commented 5 years ago

Using your patch and a modified bitbake recipe, this builds for arm successfully.

When I do python3 -c "import crc32c" I get:

ImportError: 

SSE4.2 extensions providing a crc32c hardware instruction are not available in
your processor. If you still need to use the crc32c checksum algorithm this
package comes with a software implementation that can be loaded instead. For
that set the CRC32C_SW_MODE environment variable before loading the package to
one of the following values:

 * 'auto' to use software implementation if no CPU hardware support is found.
 * 'force' to use software implementation regardless of CPU hardware support.
 * '1' means 'force', but will be eventually discontinued.

Which goes away when I set CRC32C_SW_MODE.

I've only tried importing on arm. Let me know if there's a python command I should try running.

rtobar commented 5 years ago

Thanks for letting me know that it works with the alternative solution. I take it you don't have a problem with me using that instead of yours then.

Regarding the ImportError, as the error says this is fully intended as things currently stand. The initial package implementation only exposed the hardware-accelerated crc32c instructions of Intel processors, and has thus always failed to import if such instructions were not present. A software implementation was added later, but the default importing behavior stayed to avoid giving the correct impression to users, who may think they are getting hardware acceleration in situations they aren't.

I think you would agree with me that the discussion about this importing behavior is separate from this pull request though, so I'll be closing this now.

eigendude commented 5 years ago

to avoid giving the correct impression to users, who may think they are getting hardware acceleration in situations they aren't

This is exactly how it should be.

Thanks for helping me get cross compiling working!