Revamp the python build system

Replace the setuptools based build system with scikit-build [1]. Scikit-build is an upstream python-cmake integration package that effectively enables building the extension code with cmake, rather than setuptools. The big problem with setuptools is developer experience, as it's pretty clunky and behaves quite differently from what you would expect with a more sophisticated build system for C++ code. By moving that responsibility to cmake and having skbuild invoke it, the extension code can use all of cmake for defines, compiler flags and even extension specific dependencies, without having to manually do discovery logic and configuration (essentially what cmake already does) in python. The CI definitions have been updated to reflect the changes in behaviour. There is a bug in the current upstream release 0.9.0 that makes appveyor break, but a change [2] is proposed upstream. For now, appveyor uses the jokva/scikit-build fork. For cmake .. && make && make install users, this patch should have little effect, but in all the new build system gives greater opportunity to tune and configure the compilation of the python extension. [1] https://scikit-build.readthedocs.io/en/latest [2] scikit-build/scikit-build#400
equinor · Apr 22, 2019 · f17221f · f17221f
1 parent 447b2e3
commit f17221f
Show file tree

Hide file tree

Showing 7 changed files with 146 additions and 123 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -70,15 +70,17 @@ before_install:
     - before_install
 
 install:
-    - pip install bandit
-                  numpy
-                  setuptools
-                  setuptools_scm
-                  pytest
-                  pytest-runner
-                  pybind11
-                  hypothesis
-                  sphinx
+    - pip install --upgrade
+        bandit
+        numpy
+        setuptools
+        setuptools_scm
+        pytest
+        pytest-runner
+        pybind11
+        hypothesis
+        sphinx
+        scikit-build
     - bandit -c bandit.yml -r python/
 
 before_script:
@@ -92,19 +94,15 @@ before_script:
                   ..
     - popd
 
-# distutils/setuptools on macos ignores the --rpath argument, so set
-# DYLD_LIBRARY_PATH so that the freshly-built image is picked up on for the
-# tests.
-#
-# the build is configured with CMAKE_INSTALL_NAME_DIR so that in the wheel
-# build on OS X, the delocate tool can figure out what dylib to include.
 script:
     - pushd build
-    - export DYLD_LIBRARY_PATH=$PWD/lib
     - make
     - ctest --output-on-failure
     - sudo make install
     - popd
+    # install works, so remove the _skbuild because it having root permissions
+    # from make install breaks build_wheel
+    - sudo rm -rf python/_skbuild
     - build_wheel python $PLAT
     - install_run $PLAT
     - mv wheelhouse python/dist

diff --git a/appveyor.yml b/appveyor.yml
@@ -27,18 +27,20 @@ matrix:
 install:
     - IF DEFINED PYTHON (IF "%platform%" == "x64" SET PYTHON=%PYTHON%-x64)
     - IF DEFINED PYTHON SET PATH=%PYTHON%;%PYTHON%\Scripts;%PATH%
-    - IF DEFINED PYTHON pip install numpy
-                                    pytest
-                                    pytest-runner
-                                    setuptools
-                                    setuptools_scm
-                                    pybind11
-                                    hypothesis
-                                    twine
-                                    wheel
+    - IF DEFINED PYTHON pip install --upgrade
+        numpy
+        pytest
+        setuptools
+        setuptools_scm
+        pybind11
+        hypothesis
+        twine
+        wheel
+        git+https://github.com/jokva/scikit-build
 
 before_build:
-    - IF "%platform%" == "x64" set W64="-GVisual Studio 14 2015 Win64"
+    - set generator="Visual Studio 14 2015"
+    - IF "%platform%" == "x64" set generator="Visual Studio 14 2015 Win64"
     - git submodule update --init --recursive
     - git fetch --tags
 
@@ -47,16 +49,14 @@ build_script:
           mkdir build
           pushd build
     - cmake %APPVEYOR_BUILD_FOLDER%
-            %W64%
+            -G %generator%
             -DCMAKE_CXX_FLAGS="/D_CRT_SECURE_NO_WARNINGS /EHsc"
-    - cmake --build .
-            --config %configuration%
-    - ctest --build-config %configuration%
-            --output-on-failure
+    - cmake --build .  --config %configuration%
+    - ctest --build-config %configuration% --output-on-failure
     - ps: popd
     - ps: pushd python
     - git describe
-    - IF DEFINED PYTHON python setup.py bdist_wheel
+    - IF DEFINED PYTHON python setup.py bdist_wheel -G %generator%
     - ps: popd
 
 before_deploy:

diff --git a/config.sh b/config.sh
@@ -8,7 +8,7 @@ function run_tests {
 function pre_build {
     if [ -d build-centos5 ]; then return; fi
 
-    python -m pip install cmake pybind11
+    python -m pip install cmake pybind11 scikit-build
 
     mkdir build-centos5
     pushd build-centos5

diff --git a/python/CMakeLists.txt b/python/CMakeLists.txt
@@ -1,3 +1,13 @@
+if (SKBUILD)
+    # invoked as a part of scikit-build, so this is just a proxy for the python
+    # extension cmake. this works around the fundamental limitation in cmake
+    # that it looks only for directories with a CMakeLists.txt in it, not for a
+    # named file
+    include(setup-CMakeLists.txt)
+    return ()
+endif ()
+
+cmake_minimum_required(VERSION 3.5.0)
 project(dlisio-python)
 
 if(NOT BUILD_PYTHON)
@@ -9,22 +19,21 @@ find_package(PythonInterp REQUIRED)
 if(NOT PYTHON_EXECUTABLE)
     message(WARNING "Could not find python - skipping python bindings")
     message(WARNING "Select specific python distribution with "
-                    "-DPYTHON_EXECUTABLE=bin/python"
-    )
+                    "-DPYTHON_EXECUTABLE=bin/python")
     return()
 endif()
 
 set(python ${PYTHON_EXECUTABLE})
 set(setup.py ${CMAKE_CURRENT_SOURCE_DIR}/setup.py)
 
-if (NOT WIN32)
-    # setuptools on microsoft compilers doesn't support the --library-dir or
-    # --build-dir flag and crashes, so only pass it on non-microsoft platforms
-    set(build_ext_args --library-dirs ${CMAKE_CURRENT_SOURCE_DIR}
-                       --rpath ${CMAKE_CURRENT_SOURCE_DIR}
+if (CMAKE_BUILD_TYPE)
+    # use the cmake_build_type of the source project, unless it has been
+    # specifically overriden
+    set(DLISIO_PYTHON_BUILD_TYPE
+        --build-type=${CMAKE_BUILD_TYPE}
+        CACHE STRING "override CMAKE_BUILD_TYPE in python extension"
     )
-endif()
-
+endif ()
 
 add_custom_target(
     dlisio-python ALL
@@ -34,41 +43,48 @@ add_custom_target(
     VERBATIM
     WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
 
-    # setuptools on windows breaks spectacularly when the library isn't
-    # available in the same directory, and build_ext --library-dirs is not
-    # support on msvc is not supported, so we must copy out the libsegyio core
-    # object and put it here
-    COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_FILE:dlisio>
-                                     $<TARGET_FILE_NAME:dlisio>
-
-    COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_LINKER_FILE:dlisio>
-                                     $<TARGET_LINKER_FILE_NAME:dlisio>
-
-    COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_FILE:dlisio-extension>
-                                     $<TARGET_FILE_NAME:dlisio-extension>
-
-    COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_LINKER_FILE:dlisio-extension>
-                                     $<TARGET_LINKER_FILE_NAME:dlisio-extension>
-
-    COMMAND ${python} ${setup.py} build_ext ${build_ext_args} build
+    COMMAND ${python} ${setup.py}
+        # build the extension inplace (really, once its built, copy it to the
+        # source tree) so that post-build, the directory can be used to run
+        # tests against
+        build_ext --inplace
+        build # setup.py build args
+            --cmake-executable ${CMAKE_COMMAND}
+            --generator ${CMAKE_GENERATOR}
+            ${DLISIO_PYTHON_BUILD_TYPE}
+        -- # cmake to the extension
+            -Ddlisio_DIR=${DLISIO_LIB_BINARY_DIR}
+            # "install" to the python/dlisio dir with rpath, so there's no need
+            # to fiddle with environment in ctest to load the core library from
+            # the build tree
+            -DCMAKE_INSTALL_RPATH_USE_LINK_PATH=ON
+            -DCMAKE_INSTALL_RPATH=$<TARGET_FILE_DIR:dlisio>
+            -DCMAKE_INSTALL_NAME_DIR=$<TARGET_FILE_DIR:dlisio>
 )
 
 add_dependencies(dlisio-python dlisio dlisio-extension)
 
 install(CODE "
-if (DEFINED ENV{DESTDIR})
-    get_filename_component(abs-destdir \"\$ENV{DESTDIR}\" ABSOLUTE)
-    set(root_destdir --root=\${abs-destdir})
-endif()
+    if (DEFINED ENV{DESTDIR})
+        get_filename_component(abs-destdir \"\$ENV{DESTDIR}\" ABSOLUTE)
+        set(root_destdir --root \${abs-destdir})
+    endif()
 
-execute_process(COMMAND
-    ${python} ${setup.py}
-        install --prefix=${CMAKE_INSTALL_PREFIX}
-                --single-version-externally-managed
-                --record installed-files
+    execute_process(
+        COMMAND ${python} ${setup.py}
+            install
                 \${root_destdir}
+                --single-version-externally-managed
+                --record record.txt
+                --cmake-executable \"${CMAKE_COMMAND}\"
+                --generator \"${CMAKE_GENERATOR}\"
+                ${DLISIO_PYTHON_BUILD_TYPE}
+            --
+                -Ddlisio_DIR=${DLISIO_LIB_BINARY_DIR}
+                -DCMAKE_INSTALL_RPATH_USE_LINK_PATH=OFF
         WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
-)")
+    )"
+)
 
 option(BUILD_PYDOC "Build python documentation" OFF)
 
@@ -107,10 +123,15 @@ if(BUILD_PYDOC AND sphinx)
         DESTINATION ${CMAKE_INSTALL_DATADIR}/doc/dlisio
     )
 endif()
+
 # run tests with setup.py test
 # this is very slow compared to invoking pytest directly, but setuptools will
 # copy the built extension into the tree as it sees fit
+#
+# use --skip-cmake, otherwise running the tests would trigger a build with
+# different args to setup.py, rebuilding the python lib (and wrongly so as it
+# either won't find dlisio or picked up on a system installed one)
 add_test(NAME python.unit
-         COMMAND ${python} ${setup.py} test
-         WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+    COMMAND ${python} ${setup.py} --skip-cmake test
+    WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
 )
diff --git a/python/setup-CMakeLists.txt b/python/setup-CMakeLists.txt
@@ -0,0 +1,27 @@
+cmake_minimum_required(VERSION 3.5.0)
+project(dlisio-python-extension LANGUAGES C CXX)
+
+set(CMAKE_CXX_STANDARD 11)
+set(CMAKE_CXX_VISIBILITY_PRESET "hidden")
+set(CMAKE_C_VISIBILITY_PRESET   "hidden")
+
+find_package(PythonExtensions REQUIRED)
+find_package(dlisio REQUIRED)
+
+add_library(core MODULE dlisio/ext/core.cpp)
+target_include_directories(core
+    PRIVATE
+        ${PYBIND11_INCLUDE_DIRS}
+)
+python_extension_module(core)
+target_link_libraries(core dlisio dlisio-extension)
+
+if (MSVC)
+    target_compile_options(core
+        BEFORE
+        PRIVATE
+            /EHsc
+    )
+endif ()
+
+install(TARGETS core LIBRARY DESTINATION dlisio)
diff --git a/python/setup.py b/python/setup.py
@@ -1,9 +1,8 @@
 #!/usr/bin/env python3
 
 import os
-from setuptools import setup, Extension
-from setuptools.command.build_ext import build_ext
-
+import skbuild
+import setuptools
 
 class get_pybind_include(object):
     def __init__(self, user=False):
@@ -14,32 +13,9 @@ def __str__(self):
         import pybind11
         return pybind11.get_include(self.user)
 
-
-class BuildExt(build_ext):
-    """
-    A custom build extension for adding compiler-specific, taken from
-    https://github.com/pybind/python_example/blob/master/setup.py
-    """
-    c_opts = {
-        'msvc': ['/EHsc'],
-        'unix': ['-std=c++11'],
-    }
-
-    def build_extensions(self):
-        ct = self.compiler.compiler_type
-        opts = self.c_opts.get(ct, [])
-
-        distver = self.distribution.get_version()
-        if ct == 'unix':
-            opts.append('-DVERSION_INFO="{}"'.format(distver))
-            opts.append('-fvisibility=hidden')
-        elif ct == 'msvc':
-            opts.append('/DVERSION_INFO=\\"{}\\"'.format(distver))
-
-        for ext in self.extensions:
-            ext.extra_compile_args = opts
-        build_ext.build_extensions(self)
-
+def src(x):
+    root = os.path.dirname( __file__ )
+    return os.path.abspath(os.path.join(root, x))
 
 def getversion():
     pkgversion = { 'version': '0.0.0' }
@@ -48,8 +24,10 @@ def getversion():
     if not os.path.exists(versionfile):
         return {
             'use_scm_version': {
-                'relative_to' : os.path.dirname(os.path.abspath(__file__)),
-                'write_to'    : versionfile
+                # look for git in ../
+                'relative_to' : src(''),
+                # write to ./python
+                'write_to'    : os.path.join(src(''), versionfile),
             }
         }
 
@@ -64,37 +42,39 @@ def getversion():
 
     return pkgversion
 
+pybind_includes = [
+    str(get_pybind_include()),
+    str(get_pybind_include(user = True))
+]
 
-setup(
+skbuild.setup(
     name = 'dlisio',
     description = 'DLIS v1',
     long_description = 'DLIS v1',
     url = 'https://github.com/equinor/dlisio',
     packages = ['dlisio', 'dlisio.objects'],
     license = 'LGPL-3.0',
-    ext_modules = [
-        Extension('dlisio.core',
-            sources = [
-                'dlisio/ext/core.cpp',
-            ],
-            include_dirs = ['../lib/include',
-                            '../lib/extension',
-                            '../external/mpark',
-                            '../external/mio',
-                            get_pybind_include(),
-                            get_pybind_include(user=True),
-            ],
-            libraries = ['dlisio', 'dlisio-extension'],
-        )
-    ],
     platforms = 'any',
     install_requires = ['numpy'],
     setup_requires = ['setuptools >= 28',
-                      'pytest-runner',
                       'pybind11 >= 2.2',
                       'setuptools_scm',
+                      'pytest-runner',
     ],
     tests_require = ['pytest'],
-    cmdclass = { 'build_ext': BuildExt },
+    # we're building with the pybind11 fetched from pip. Since we don't rely on
+    # a cmake-installed pybind there's also no find_package(pybind11) -
+    # instead, the get include dirs from the package and give directly from
+    # here
+    cmake_args = [
+        '-DPYBIND11_INCLUDE_DIRS=' + ';'.join(pybind_includes),
+        # we can safely pass OSX_DEPLOYMENT_TARGET as it's ignored on
+        # everything not OS X. We depend on C++11, which makes our minimum
+        # supported OS X release 10.9
+        '-DCMAKE_OSX_DEPLOYMENT_TARGET=10.9',
+    ],
+    # skbuild's test imples develop, which is pretty obnoxious instead, use a
+    # manually integrated pytest.
+    cmdclass = { 'test': setuptools.command.test.test },
     **getversion()
 )