Build project using scikit-build #113
Conversation
|
Well, that was surprisingly easy! I modified both What's left is
|
astrojuanlu
force-pushed
the
scikit-build
branch
from
a27de37
to
1f77448
Compare
astrojuanlu
force-pushed
the
scikit-build
branch
from
1f77448
to
dd74ab2
Compare
astrojuanlu
force-pushed
the
scikit-build
branch
from
dd74ab2
to
6fbb048
Compare
astrojuanlu
changed the title
|
@bluescarni @darioizzo this is ready for review. |
astrojuanlu
force-pushed
the
scikit-build
branch
from
6feef41
to
e48e1fc
Compare
|
The CI failures are caused by an import error because of the current working directory. Fixing. |
|
There are 5 remaining test failures:
|
|
just FYI, I am getting the same errors in the build on PR #112 despite it building and running the tests fine on my machine. I'm also in the dark as to what is causing the issue |
|
@astrojuanlu Thanks for taking the time to look into this. As a general comment, the scikit approach should be an alternative to the CMake build sytem and not a replacement for it. Thus, I would ask if you could place the scikit instructions in a new paragraph in the docs, rather than modifying the existing CMake instructions. Similarly, the existing conda based builds in the CI pipeline should remain unaltered and new builds testing the scikit approach should be added instead (preferably on github actions, as I think we may be hitting the limits for open source projects on other CI providers). I'll leave a couple more comments on the diff. |
| @@ -140,8 +140,6 @@ endif() | |||
| find_package(Python3 REQUIRED COMPONENTS Interpreter Development) | |||
| message(STATUS "Python3 interpreter: ${Python3_EXECUTABLE}") | |||
| message(STATUS "Python3 installation directory: ${Python3_SITEARCH}") | |||
| set(PYGMO_INSTALL_PATH "" CACHE STRING "pygmo module installation path") | |||
There was a problem hiding this comment.
This is a useful (albeit niche) option. It would be better if we could keep it. What problems does it cause with scikit?
| @@ -1,20 +1,6 @@ | |||
| # Configure the version file. | |||
| configure_file("${CMAKE_CURRENT_SOURCE_DIR}/_version.py.in" "${CMAKE_CURRENT_BINARY_DIR}/_version.py" @ONLY) | |||
|
|
|||
| # Configure the files needed to make the python wheels (for PyPi packages) | |||
| requires = [ | ||
| "setuptools>=42", | ||
| "scikit-build", | ||
| "pybind11", |
There was a problem hiding this comment.
What is the rationale for adding pybind11 here? Shouldn't we expect pybind11 to be installed with the other of C++ dependencies?
| from setuptools.dist import Distribution | ||
| import sys | ||
|
|
||
| NAME = 'pygmo' | ||
| VERSION = '@pygmo_VERSION@' |
There was a problem hiding this comment.
It's not ideal to have to remember to update the version number of pygmo in several different places when making new releases. I'd propose to have a simple txt file in the root directory of the project called pygmo_version containing just 2.18.0 (say) and then reading the version number from this file from both CMake and setup.py.
|
closing this #117 substitutes it up to when we will consider again scikit-build-core |
This attempts to use scikit-build to build pygmo in an effort to simplify gh-110. For that, I copied
tools/wheel_setup.pytosetup.pyand made some minimal modifications.I got surprisingly far with comparatively low effort, although the PR is not finished yet:
setup.py. With the old process CMake takes care of configuring that - I'm wondering if we could do the same using scikit-build.PYGMO_INSTALL_PATHso thatpip install .works on the environment I was using, but using this absolute path is brittle. I am investigating whether a more generic solution can be implemented.I tested this in a conda environment with the released version of pagmo, which means that I had to temporarily revert 97770b2. However, I am confident it will work with development versions of pagmo as well.