Skip to content

Conversation

@ktbarrett
Copy link
Contributor

@ktbarrett ktbarrett commented Aug 6, 2020

This advanced example, unlike the other examples, builds a separate shared library and installs a CMake package to use that shared library within the Python package. The example uses pybind11 to implement the C++-Python binding, but it's not a pybind11 best practices example.

The example will contain both Python tests, and a C++ test that consumes the C++ library using the CMake package as installed into the Python package directory.

I'm open to name changes, code formatting changes, anything, as long as we drive best practices home.

This was done in reaction to the discussion in #6. Pinging @henryiii @phcerdan @jcfr.

TODO

  • Add C++ test project

@ktbarrett ktbarrett marked this pull request as ready for review August 7, 2020 03:33
@ktbarrett
Copy link
Contributor Author

I will also add a __main__ to the package so we can set hello_ROOT in the c++ test instead of the current method.

@henryiii
Copy link
Contributor

that's a misconfigured environment

AKA Windows...

@ktbarrett
Copy link
Contributor Author

Did I stutter? 😉 It is updated with a minimal example of an entry point configured with the top level CMake file to correctly point to the installed CMake package files.

@ktbarrett
Copy link
Contributor Author

@henryiii I updated to use ${PYTHON_EXECUTABLE} -m pybind11 this is ready to go.

@henryiii henryiii self-requested a review March 30, 2021 16:57
@henryiii
Copy link
Contributor

henryiii commented Mar 30, 2021

I'll try to look at this soon, ping me if I don't get to it this week!

Copy link
Contributor

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

A few very minor suggestions.

Comment on lines 81 to 84
# call pybind11-config to obtain the root of the cmake package
execute_process(COMMAND ${PYTHON_EXECUTABLE} -m pybind11 --cmakedir OUTPUT_VARIABLE pybind11_ROOT_RAW)
string(STRIP ${pybind11_ROOT_RAW} pybind11_ROOT)
find_package(pybind11)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm hoping to remove the need to do this eventually. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How's that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Current idea is to add an entry point mechanism to scikit-build. Pybind11 would define the entrypoint:

[options.entry_points]
scikit-build.module =
  module=pybind11.skbuild:module

The "module" function returns a path pointing to the CMake files in the package. This is added to CMake’s search path by scikit-build, iterating over all matching entry points.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm... I'm not familiar with the tooling around discovering entry points, but it sounds reasonable. Are those CMake files to be added via something akin to add_subdirectory? I have a project I'm writing in C++, but also want to distribute a Python wrapper (using pybind11) and distribute the C++ libraries, CMake to consume those libraries, and the Python wrapper using pip.

I did make the changes suggested. Are we waiting on @jcfr to do a final review and pull?

@ktbarrett
Copy link
Contributor Author

Output on my machine.
(base) ~/dev/scikit-build-sample-projects/projects/hello-cmake-package advanced-example
> pip install .
/home/kaleb/miniconda3/lib/python3.8/site-packages/secretstorage/dhcrypto.py:16: CryptographyDeprecationWarning: int_from_bytes is deprecated, use int.from_bytes instead
  from cryptography.utils import int_from_bytes
/home/kaleb/miniconda3/lib/python3.8/site-packages/secretstorage/util.py:25: CryptographyDeprecationWarning: int_from_bytes is deprecated, use int.from_bytes instead
  from cryptography.utils import int_from_bytes
Processing /home/kaleb/dev/scikit-build-sample-projects/projects/hello-cmake-package
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
    Preparing wheel metadata ... done
Building wheels for collected packages: hello-cmake-package
  Building wheel for hello-cmake-package (PEP 517) ... done
  Created wheel for hello-cmake-package: filename=hello_cmake_package-0.1.0-cp38-cp38-linux_x86_64.whl size=51150 sha256=6a45cb9e899f43a545e0de7c54330fcf58000dd76b01f780c2428a6fdc96958c
  Stored in directory: /home/kaleb/.cache/pip/wheels/23/15/4b/8ff68df4eb557c32d731383ed549dc9d6843d8379f31a50bbd
Successfully built hello-cmake-package
Installing collected packages: hello-cmake-package
Successfully installed hello-cmake-package-0.1.0

(base) ~/dev/scikit-build-sample-projects/projects/hello-cmake-package advanced-example
> pytest
================================================================================================================================= test session starts ==================================================================================================================================
platform linux -- Python 3.8.8, pytest-6.2.2, py-1.9.0, pluggy-0.13.1 -- /home/kaleb/miniconda3/bin/python
cachedir: .pytest_cache
rootdir: /home/kaleb/dev/scikit-build-sample-projects, configfile: pytest.ini
plugins: forked-1.3.0, xdist-2.1.0, cov-2.10.1, profiling-1.7.0
collected 2 items

tests/test_hello.py::test_hello PASSED                                                                                                                                                                                                                                           [ 50%]
tests/test_hello.py::test_return_two PASSED                                                                                                                                                                                                                                      [100%]

----------- coverage: platform linux, python 3.8.8-final-0 -----------
Coverage XML written to file coverage.xml


================================================================================================================================== 2 passed in 0.04s ===================================================================================================================================

(base) ~/dev/scikit-build-sample-projects/projects/hello-cmake-package advanced-example
> cmake -S tests/cpp/ -B build -Dhello_ROOT=$(python -m hello --cmakefiles)
-- The C compiler identification is GNU 9.3.0
-- The CXX compiler identification is GNU 9.3.0
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: /home/kaleb/dev/scikit-build-sample-projects/projects/hello-cmake-package/build

(base) ~/dev/scikit-build-sample-projects/projects/hello-cmake-package advanced-example
> cmake --build build/
Scanning dependencies of target hello_user
[ 50%] Building CXX object CMakeFiles/hello_user.dir/test.cpp.o
[100%] Linking CXX executable hello_user
[100%] Built target hello_user

(base) ~/dev/scikit-build-sample-projects/projects/hello-cmake-package advanced-example
> cd build/

(base) ~/dev/scikit-build-sample-projects/projects/hello-cmake-package/build advanced-example
> ctest
Test project /home/kaleb/dev/scikit-build-sample-projects/projects/hello-cmake-package/build
    Start 1: hello_test
1/1 Test #1: hello_test .......................   Passed    0.00 sec

100% tests passed, 0 tests failed out of 1

Total Test time (real) =   0.00 sec

(base) ~/dev/scikit-build-sample-projects/projects/hello-cmake-package/build advanced-example

ktbarrett and others added 5 commits April 23, 2021 17:18
This advanced example, unlike the other examples, builds a separate
shared library and installs a CMake package to use that shared library
with the Python package.

The example uses pybind11 to implement the C++-Python binding.
The example has Python tests, but is missing C++ tests.
@henryiii
Copy link
Contributor

Tests pass too, so I'm happy.

@henryiii henryiii closed this Apr 28, 2021
@henryiii henryiii reopened this Apr 28, 2021
@henryiii
Copy link
Contributor

Any idea how to fix this on Windows? hello.lib is not available.

@ktbarrett
Copy link
Contributor Author

I suppose I'd have to figure out how to generate and install import libraries on Windows in CMake. I'll get around to it.

@ktbarrett
Copy link
Contributor Author

Considering the date I last left a comment and when I first did the work, you can consider this abandoned. I'm not particularly knowledgeable about building on Windows. I think this example is worthwhile, perhaps even if it doesn't work with Windows yet.

@henryiii
Copy link
Contributor

I"m fine to drop it in as UNIX only, I'd just need to add an if into the noxfile, I think. It should be pretty easy to make it work if someone knew more about Windows building than I do. :)

@henryiii henryiii merged commit 8af3bb1 into scikit-build:master Feb 15, 2022
@ktbarrett ktbarrett deleted the advanced-example branch August 17, 2022 22:06
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.

2 participants