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

Improved cross-compilation support #5083

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

tttapa
Copy link
Contributor

@tttapa tttapa commented Apr 1, 2024

Description

This PR aims to improve cross-compilation support:

  • When cross-compiling, find_package(Python) is now called without the Interpreter component.
  • CMake code that uses execute_process(COMMAND ${Python_EXECUTABLE} ...) has been guarded by if(NOT CMAKE_CROSSCOMPILING).
  • If the user did not specify the required pybind11-specific variables PYTHON_IS_DEBUG, PYTHON_MODULE_EXTENSION and PYTHON_MODULE_DEBUG_POSTFIX, the new CMake code attempts to deduce sensible values from the SETUPTOOLS_EXT_SUFFIX environment variable (set by e.g. cibuildwheel) and the Python_SOABI variable provided by FindPython (CMake >= 3.17).
  • If none of those variables are set, a clear error message is presented to the user, asking them to provide the necessary information explicitly (rather than failing when trying to query the Python interpreter).

Suggested changelog entry:

No longer rely on the Python interpreter when cross-compiling

Details

(The diffs look larger than they are, mostly because of whitespace changes from wrapping blocks in if statements, see https://github.com/pybind/pybind11/pull/5083/files?diff=unified&w=1 for a diff that ignores whitespace.)

FindPython

To ensure consistency between the Interpreter and Development.Module components, the call to find_package(Python) should generally include both components simultaneously. However, this is not true when cross-compiling: finding the Interpreter component will fail because CMake cannot run the binary for the target on the host.

https://gitlab.kitware.com/cmake/cmake/-/issues/25145#note_1396533
For external projects that call find_package(Python ... Interpreter Development), you should file a bug with upstream, telling them to separate their host and cross builds in one of the ways that @marc.chevrier suggested, or apply a local patch that does this. find_package(Python ... Interpreter Development) with a native interpreter and a cross library is never going to be supported.

SETUPTOOLS_EXT_SUFFIX

This environment variable is set by cibuildwheel when cross-compiling on Windows. I chose to let this have precedence over the Python_SOABI variable, under the assumption that cibuildwheel has the necessary information to get this value correct.

Python_SOABI

The Python_SOABI variable is available in CMake 3.17 and later. If it is unavailable and no other way to determine the extension suffix is found, the user will be asked to provide the necessary information directly (by setting the PYTHON_IS_DEBUG, PYTHON_MODULE_EXTENSION and PYTHON_MODULE_DEBUG_POSTFIX variables).

Caching

Since we don't need to query Python for information, the result variables are not added to the CMake cache. If the SETUPTOOLS_EXT_SUFFIX environment variable is used, a cache entry PYTHON_MODULE_EXT_SUFFIX is created to save its value for subsequent runs.

Native builds

Native builds are not affected by this PR, all new logic is guarded by if(CMAKE_CROSSCOMPILING).

Tests

I've added a file tools/test-pybind11GuessPythonExtSuffix.cmake which checks that the PYTHON_MODULE_EXTENSION/PYTHON_MODULE_DEBUG_POSTFIX variables are set correctly for different values of Python_SOABI on Windows, macOS and Linux.
It has not yet been integrated in the rest of the test suite (pointers on how best to do this are welcome).

I've created a repository (tttapa/pybind11-cross-test) with minimal CMake toolchain files for cross-compiling a simple pybind11 extension module for ARM64 on Windows, Linux and macOS. All CI runs are successful, and the correct extension suffixes are used: https://github.com/tttapa/pybind11-cross-test/actions/runs/8511097705/

Quirks

Linux:
CMake's FindPython does not correctly find the SOABI for Python 3.6 and Python 3.7 (it is set to the empty string), unless Python_FIND_ABI="ANY;ANY;ANY" is specified. Annoyingly, setting Python_FIND_ABI breaks things for Python >=3.8, but I believe that this is something that needs to be fixed upstream.

Windows:
It seems that FindPython does not find the SOABI at all. This is why SETUPTOOLS_EXT_SUFFIX is necessary. Even if this environment variable is not set, the proposed code falls back to .pyd as the extension suffix when Python_SOABI is set to the empty string. This again seems like a limitation of FindPython itself and not something that can be fixed in pybind11.

@tttapa tttapa requested a review from henryiii as a code owner April 1, 2024 16:50
@henryiii
Copy link
Collaborator

henryiii commented Apr 3, 2024

I think this is the right direction, but I'm curious, what would a tool like scikit-build-core set here to ensure the correct Python_SOABI is set? Can you just set Python_SOABI if not finding Interpreter?

@tttapa
Copy link
Contributor Author

tttapa commented Apr 3, 2024

If scikit-build-core knows the desired extension suffix and ABI, it should set the PYTHON_IS_DEBUG, PYTHON_MODULE_EXTENSION and PYTHON_MODULE_DEBUG_POSTFIX CMake variables. In that case, the CMake side doesn't do any guessing and doesn't access Python_SOABI, and these values are used directly by the existing pybind11 CMake scripts.
But scikit-build-core should of course make sure that these values actually correspond to the version of Python that is found by CMake, otherwise this will silently generate invalid extension modules.

Alternatively, to be less pybind11-specific, it could set ENV{SETUPTOOLS_EXT_SUFFIX} (or assume that it is set by the user), which is what cibuildwheel does now.

On macOS and Linux, with sufficiently recent CMake and Python >=3.8, it doesn't need to set anything, simply pointing to the desired Python installation using Python_ROOT_DIR (and making sure that it's under the CMAKE_FIND_ROOT_PATH) should be sufficient.

IIRC, FindPython unconditionally unsets Python_SOABI, so setting that explicitly from the command line probably won't work.


The way I currently approach this in py-build-cmake is as follows:

  1. If the user provides a cross-compilation configuration file, that takes precedence. The configuration contains the Python version and ABI (used for the wheel filename), and the Python root folder that is passed to CMake. The SOABI is then determined by FindPython. The user is free to add CMake cache variables if they need to (e.g. pybind11's PYTHON_MODULE_EXTENSION), but this is their responsibility, and so is making sure that the config they provide is consistent (i.e. the Python version matches the actual version installed in the provided Python root folder).
    I also sometimes manually provide the FindPython hints in my CMake toolchain file.
  2. If the user does not provide a cross-compilation configuration, py-build-cmake checks the environment for things like DIST_EXTRA_CONFIG, SETUPTOOLS_EXT_SUFFIX and _PYTHON_HOST_PLATFORM, and automatically configures cross-compilation in a way that is compatible with cibuildwheel.

It would be nice if we could come up a more robust, standardized way, though :)

@henryiii
Copy link
Collaborator

I'd like to get this in for the next release, but I'm worried about potential regressions, especially for cross-compilation that currently work, like WebAssembly. So I think I'll make it opt-in with a variable like PYBIND11_CROSSCOMPILE for now, then we can see if we can remove the need for the variable in the future.

@tttapa tttapa force-pushed the cross branch 2 times, most recently from 14ad704 to 12159d5 Compare May 29, 2024 20:58
@henryiii
Copy link
Collaborator

henryiii commented May 29, 2024

I just did this too. The main differences: I used PYBIND11_USE_CROSSCOMPILING instead of PYBIND11_CROSSCOMPILING, and that just opts into the new behavior. Internally CMAKE_CROSSCOMPILING sets _PYBIND11_CROSSCOMPILING if PYBIND11_USE_CROSSCOMPILING is on. I don't have a warning yet about the change, I'd like to save that for a version I think until we see the impact.

Thoughts?

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 18814d4d..3526a1a6 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -116,6 +116,7 @@ option(PYBIND11_NUMPY_1_ONLY
 set(PYBIND11_INTERNALS_VERSION
     ""
     CACHE STRING "Override the ABI version, may be used to enable the unstable ABI.")
+option(PYBIND11_USE_CROSSCOMPILING "Respect CMAKE_CROSSCOMPILING" OFF)
 
 if(PYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION)
   add_compile_definitions(PYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION)
diff --git a/tools/FindPythonLibsNew.cmake b/tools/FindPythonLibsNew.cmake
index 8275b9d5..0640a191 100644
--- a/tools/FindPythonLibsNew.cmake
+++ b/tools/FindPythonLibsNew.cmake
@@ -205,7 +205,7 @@ endif()
 # Make sure the Python has the same pointer-size as the chosen compiler
 # Skip if CMAKE_SIZEOF_VOID_P is not defined
 # This should be skipped for (non-Apple) cross-compiles (like EMSCRIPTEN)
-if(NOT CMAKE_CROSSCOMPILING
+if(NOT _PYBIND11_CROSSCOMPILING
    AND CMAKE_SIZEOF_VOID_P
    AND (NOT "${PYTHON_SIZEOF_VOID_P}" STREQUAL "${CMAKE_SIZEOF_VOID_P}"))
   if(PythonLibsNew_FIND_REQUIRED)
diff --git a/tools/pybind11Common.cmake b/tools/pybind11Common.cmake
index 06ee9253..d8e18e67 100644
--- a/tools/pybind11Common.cmake
+++ b/tools/pybind11Common.cmake
@@ -42,6 +42,16 @@ set(pybind11_INCLUDE_DIRS
     "${pybind11_INCLUDE_DIR}"
     CACHE INTERNAL "Include directory for pybind11 (Python not requested)")
 
+if(CMAKE_CROSSCOMPILING AND PYBIND11_USE_CROSSCOMPILING)
+  set(_PYBIND11_CROSSCOMPILING
+      ON
+      CACHE INTERNAL "")
+else()
+  set(_PYBIND11_CROSSCOMPILING
+      OFF
+      CACHE INTERNAL "")
+endif()
+
 # --------------------- Shared targets ----------------------------
 
 # Build an interface library target:
@@ -195,7 +205,7 @@ endif()
 
 # --------------------- pybind11_find_import -------------------------------
 
-if(NOT _pybind11_nopython AND NOT CMAKE_CROSSCOMPILING)
+if(NOT _pybind11_nopython AND NOT _PYBIND11_CROSSCOMPILING)
   # Check to see if modules are importable. Use REQUIRED to force an error if
   # one of the modules is not found. <package_name>_FOUND will be set if the
   # package was found (underscores replace dashes if present). QUIET will hide
diff --git a/tools/pybind11NewTools.cmake b/tools/pybind11NewTools.cmake
index dc3adfe9..f2ec3475 100644
--- a/tools/pybind11NewTools.cmake
+++ b/tools/pybind11NewTools.cmake
@@ -33,7 +33,7 @@ if(NOT Python_FOUND AND NOT Python3_FOUND)
   endif()
 
   # Interpreter should not be found when cross-compiling
-  if(CMAKE_CROSSCOMPILING)
+  if(_PYBIND11_CROSSCOMPILING)
     set(_pybind11_interp_component "")
   else()
     set(_pybind11_interp_component Interpreter)
@@ -110,7 +110,7 @@ if(PYBIND11_MASTER_PROJECT)
   endif()
 endif()
 
-if(NOT CMAKE_CROSSCOMPILING)
+if(NOT _PYBIND11_CROSSCOMPILING)
   # If a user finds Python, they may forget to include the Interpreter component
   # and the following two steps require it. It is highly recommended by CMake
   # when finding development libraries anyway, so we will require it.

@tttapa
Copy link
Contributor Author

tttapa commented May 29, 2024

Sure, looks good to me, feel free to replace my last commit :)

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
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.

None yet

2 participants