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

Use pybind11 with cmake_find_package only #1481

Merged

Conversation

danimtb
Copy link
Member

@danimtb danimtb commented Apr 28, 2020

Specify library name and version: pybin11/all

Try to consume pybind11 in the canonical way indicated in the documentation for CMake.

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@danimtb danimtb self-assigned this Apr 28, 2020
@conan-center-bot
Copy link
Collaborator

Some configurations of 'pybind11/2.4.3' failed in build 1 (168855fb868d16ac160d5723bce4ac674a08e2c1):

Comment on lines 12 to 11
include("${CMAKE_BINARY_DIR}/conanbuildinfo.cmake")
conan_basic_setup()
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the cmake helper always required to make sure that the compiler flags are set correctly?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not mandatory. Here I wanted to test that the package can be consumed ina transparent way with the CMake macros they provide.

Copy link
Contributor

Choose a reason for hiding this comment

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

It will be needed when a cpython recipe is used.
Can't the call to conan_basic_setup be moved to after the find_package?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, please, leave the conan_basic_setup() call, it is our toolchain until Conan provides a proper toolchain (WIP).

@conan-center-bot
Copy link
Collaborator

Some configurations of 'pybind11/2.4.3' failed in build 2 (050470a914c997b588fa092292bac1f271c070f3):

@conan-center-bot
Copy link
Collaborator

Some configurations of 'pybind11/2.4.3' failed in build 3 (ff3528f2f4b09cd08182fcc9995f9753ddee9ff7):

@conan-center-bot
Copy link
Collaborator

Some configurations of 'pybind11/2.4.3' failed in build 4 (ee030bca78f63c9f92021b881f648c9e249afc79):

@conan-center-bot
Copy link
Collaborator

All green in build 5 (53ae50cd54ff381b306c017513c7c85494f9d31e)! 😊

@conan-center-bot
Copy link
Collaborator

All green in build 6 (9d9ee45e9bcde50aa20d4c9eccecad24c56a1b17)! 😊

@conan-center-bot
Copy link
Collaborator

All green in build 7 (dec99f18caa000391fc60f0bd804e0f8f02a5695)! 😊

@conan-center-bot
Copy link
Collaborator

All green in build 8 (e9ad5c05c077eb304f6a940d8eb7f3f88b78467b)! 😊

@conan-center-bot
Copy link
Collaborator

Some configurations of 'pybind11/2.4.3' failed in build 9 (f4ce1e73ca51257b04f503369c93b7305be294c7):

@conan-center-bot
Copy link
Collaborator

Some configurations of 'pybind11/2.4.3' failed in build 10 (0e62081835b7c5bca56ed3b12ec504d122ba9fc0):

@conan-center-bot
Copy link
Collaborator

An unexpected error happened and has been reported. Help is on its way! 🏇

@conan-center-bot
Copy link
Collaborator

Some configurations of 'pybind11/2.4.3' failed in build 12 (93cd294db5c8263a73d14905b5144ecac45b3563):

@@ -38,6 +38,8 @@ def package(self):
cmake.install()
os.unlink(os.path.join(self.package_folder, "lib", "cmake", "pybind11", "pybind11Config.cmake"))
os.unlink(os.path.join(self.package_folder, "lib", "cmake", "pybind11", "pybind11ConfigVersion.cmake"))
pybind11tools_path = os.path.join(self.package_folder, "lib", "cmake", "pybind11", "pybind11Tools.cmake")
tools.replace_in_file(pybind11tools_path, "PYBIND11_INCLUDE_DIR", "pybind11_INCLUDE_DIRS")
Copy link
Member Author

Choose a reason for hiding this comment

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

@jgsogo maybe this is not a good solution when consuming this package from the cmake_package_multi generator?

Copy link
Contributor

Choose a reason for hiding this comment

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

What I see here is that inside the file pybind11Tools we have a problem. We shouldn't use pybind11_INCLUDE_DIRS but the pybind11::pybind11 target... but it won't work if using just a cmake generator.

I only have questions here: can we use the CONAN_LIB::... target? Probably not the same name for cmake_find_package generators. Should we create always the target pybind11::pybind11 even for cmake generator? What about old CMake without targets supports (do we care about so old versions for this recipe?)?

A different way to go is add a list to different include directories (it already has a bunch of them). We can add the generator expression that works for the cmake_find_package[_multi] generator:

  target_include_directories(${target_name} ${inc_isystem}
    PRIVATE ${pybind11_INCLUDE_DIRS}  # from project CMakeLists.txt
    PRIVATE ${pybind11_INCLUDE_DIR}  # from pybind11Config
    PRIVATE ${PYTHON_INCLUDE_DIRS}
)

Here we have a similar problem as the one be have in the protobuf package. We need to hardcode in a file something that works for any generator that we will define afterward. Can we workaround it? Is this one reason why we need a proper deploy function with the additional install folder in the cache?

I know this is not an answer, only questions, but I need to think more about this issue.

@ericriff
Copy link
Contributor

ericriff commented May 5, 2020

I'm trying to replace my locally built pybind11 with one provided by conan but I get linking errors regarding pybind11::module and pybind11::embed which are defined in pybind11Config.cmake.
Maybe we can fix it in this PR.

CMake Error at path/CMakeLists.txt:6 (add_library):
  Target "some-obj" links to target "pybind11::module" but the
  target was not found. 

@SSE4
Copy link
Contributor

SSE4 commented Jul 27, 2020

@danimtb pls rebase

@danimtb danimtb marked this pull request as draft July 27, 2020 15:54
@danimtb
Copy link
Member Author

danimtb commented Jul 27, 2020

@SSE4 this was just a test/proposal and it is still not ready to be merged. I have made it a draft. Thanks

@madebr
Copy link
Contributor

madebr commented Oct 17, 2020

Is this pull request still alive?

@danimtb
Copy link
Member Author

danimtb commented Oct 20, 2020

@madebr I need to give it another try. Unfortunately, we cannot remove conan_basic_setup() and the replace in file after the installation of the package is dependant on the consumer's generator.

@conan-center-bot
Copy link
Collaborator

All green in build 15 (bec34d2a4e2837b3771c73f104006f76af5e99b9)! 😊

@ericriff
Copy link
Contributor

ericriff commented Oct 23, 2020

Would it be possible to also add the targets that the documentation specifies?
https://pybind11.readthedocs.io/en/stable/cmake/index.html?highlight=cmake#exported-targets
Right now the conan packages for pybind11 are not interchangeable with the official ones because of the missing targets.

@conan-center-bot
Copy link
Collaborator

All green in build 16 (ea958b0033edcaae67449a85201e82e4fdbf650d)! 😊

@danimtb
Copy link
Member Author

danimtb commented Oct 26, 2020

This PR is not exaclty what I was expecting but at least we prove that the consumption of the library can be simplified and the macros of cmake work. If you think it is not relevant I am fine dropping it. Thanks!

@danimtb danimtb marked this pull request as ready for review October 26, 2020 08:59
Copy link
Contributor

@SSE4 SSE4 left a comment

Choose a reason for hiding this comment

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

less code is always nice!

@prince-chrismc prince-chrismc mentioned this pull request Nov 6, 2020
4 tasks
@danimtb danimtb assigned uilianries and unassigned danimtb Nov 6, 2020
@conan-center-bot
Copy link
Collaborator

All green in build 17 (17bd674e61b86484d5473e12744bb93b756ca310)! 😊

@danimtb
Copy link
Member Author

danimtb commented Nov 10, 2020

Seems everything works fine after the line endings change, sorry for the noise 😅

@Croydon
Copy link
Contributor

Croydon commented Nov 10, 2020

Wow. GitHub is not removing existing approvals when an empty commit is pushed. Very smart 👍

@conan-center-bot conan-center-bot merged commit 05d9b79 into conan-io:master Nov 12, 2020
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.

9 participants