Update to pybind11 2.6.0 #3305

kaihowl · 2020-10-24T13:36:07Z

This includes changes to proper use of the generated config from the
main pybind11 project. Since the layout of the files and their logic
slightly changed in 2.6.0 these changes were required. The changes work
for versions 2.5.0 and 2.4.3 as well.

The test_package is adapted to resemble the corresponding test case
https://github.com/pybind/pybind11/blob/master/tests/test_cmake_build/installed_target/CMakeLists.txt

Specify library name and version: pybind11/2.6.0

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.

This includes changes to proper use of the generated config from the main pybind11 project. Since the layout of the files and their logic slightly changed in 2.6.0 these changes were required. The changes work for versions 2.5.0 and 2.4.3 as well. The test_package is adapted to resemble the corresponding test case https://github.com/pybind/pybind11/blob/master/tests/test_cmake_build/installed_target/CMakeLists.txt

conan-center-bot · 2020-10-24T13:44:51Z

All green in build 1 (83a2cd1294ca1e207e537593fc986f511c015ddc)! 😊

pybind11/2.4.3: Generated 1 packages. All logs here
pybind11/2.5.0: Generated 1 packages. All logs here
pybind11/2.6.0: Generated 1 packages. All logs here

prince-chrismc · 2020-10-24T21:07:52Z

Huge thanks for bumping this project 🏆

I'm on mobile for forgive me if I didn't understand the PR.
The new cmake targets exposed my pybind should be "modeled" by the conan components feature.
It is difficult to tell (I'll have to review from my desktop) but you should not be keeping the .cmake files generated on the installation, there's a big section in the docs about this.

This PR is a huge help, I'll try to give some better pointers shortly if anything needs to be tweaked!

prince-chrismc · 2020-10-24T23:15:51Z

I understand much better what you are trying to fix, however it does clash with on of the C3i rules, about CMake config files, see here.

I've spent some times and I've sorted out (hopefully) the targets installed in two of the versions.

v2.5.0: https://github.com/pybind/pybind11/blob/3b1dbebabc801c9cf6f0953a4c20b904d444f879/CMakeLists.txt#L149

pybind11::pybind11
pybind11::module
pybind11::embed

v2.6.0: https://github.com/pybind/pybind11/blob/59a2ac2745d8a57ac94c6accced73620d59fb844/CMakeLists.txt#L224-L227

pybind11::pybind11_headers

Their setup is mighty confusing. The installed CMake script in v2.6.0 also generates more targets...
https://github.com/pybind/pybind11/blob/59a2ac2745d8a57ac94c6accced73620d59fb844/tools/pybind11NewTools.cmake#L137

pybind11::pybind11
pybind11::module
pybind11::embed

Which should look familiar to the 2.5.0 targets

🤯

Another thing which I notices was the fact the CMake scripts which have helper functions have also moved around.

kaihowl · 2020-10-25T10:45:43Z

Hey! Thank you for the quick and detailed review. Much appreciated!

The guideline to not include a package config directly but instead model things correctly in Conan makes sense. I had a hard time with the new pybind11 version to follow this advice, though. I found this prior art on directly including a package config. Since I also did not see any absolute paths in the package config, I thought that I might get away with simply renaming the original package config as well.

When I try to include pybind11NewTools.cmake instead of pybind11Tools.cmake I run into

CMake Error at /Users/kaihowl/.conan/data/pybind11/2.6.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/lib/cmake/pybind11/pybind11NewTools.cmake:8 (get_property):
  get_property could not find TARGET pybind11::headers.  Perhaps it has not
  yet been created.
Call Stack (most recent call first):
  build/e29c9ddac43fd2aed5e447c86b859223b99548aa/Findpybind11.cmake:156 (include)
  CMakeLists.txt:7 (find_package)

It seems that both the main CMakeLists.txt in the pybind11 project and the package config define this alias target (dependent on sub-project- or installed-project-usage).

Would modelling the targets defined in pybind11's cmakes with the Conan components feature help? I think these targets still need to be defined in exactly the same way as specified in the pybind11 project. I fear that this duplication is brittle and will make updating the package in the future more difficult.

Since I am not an expert in neither CMake nor in Conan, I unfortunately need more help. How do I proceed from here? Thank you again for your pointers already! :)

prince-chrismc · 2020-10-25T14:04:53Z

Would modelling the targets defined in pybind11's cmakes with the Conan components feature help?

Yes!

How do I proceed from here?

#2803 is a good example, it will allow you to create the correct cmake targets.

I suspect we will need to keep the config files (old and new) and yes we are allowed to rename them.
I would strongly recommend calling the helper functions from the test_package to make sure they work.

I am no expert myself, so I think having a few extra minds on this might help.
@uilianries @SpaceIm @madebr This project has change its cmake significantly. If you have a chance to review it, that would be appreciated 😄

SpaceIm · 2020-10-27T15:54:25Z

CMake wrapper shouldn't be removed, or if it is removed, include("${CMAKE_BINARY_DIR}/conanbuildinfo.cmake") and conan_basic_setup() should be injected in upstream's CMakeLists.txt

SpaceIm · 2020-10-27T15:56:07Z

recipes/pybind11/all/conanfile.py

+        os.rename("{}-{}".format(self.name, self.version),
+                  self._source_subfolder)


Could you restore all these changes based on length? CCI's .editorconfig allows 200 characters per line.

In the PR template I had to tick off "I've followed the PEP8 style guides for Python code in the recipes." In PEP8 it says 79 chars per line (and optionally up to 99 chars). I started using autopep8 in order to comply to this rule.
Do we have to change the PR template to reflect this? With which tool do I use an editorconfig file?

Line length is indeed an exception to the rest of PEP8 style

See https://editorconfig.org for a list of editors with native support and a list of plugins for even more editors

SpaceIm · 2020-10-27T15:59:09Z

recipes/pybind11/all/conanfile.py

+        self.cpp_info.build_modules = [get_path("FindPythonLibsNew.cmake"),
+                                       get_path("pybind11Install.cmake")]


Can't it be modeled with conan components? This block defeats CCI policy.

According to the docs, pybind11 exposes a lot of targets. https://pybind11.readthedocs.io/en/stable/compiling.html#advanced-interface-library-targets

Should all of these be modeled in the recipe?

It would be better. Those 2 custom cmake functions pybind11_strip() and pybind11_extension() are in pybind11Config.cmake?

SpaceIm · 2020-10-27T16:00:14Z

recipes/pybind11/all/test_package/conanfile.py

@@ -1,6 +1,7 @@
 from conans import ConanFile, CMake, tools
 import os
 import sys
+from platform import python_version


Doesn't seem to be used.

prince-chrismc · 2020-11-06T03:42:55Z

Saw this PR which is related #1481

prince-chrismc · 2020-11-07T04:55:18Z

might resolve #2099

andioz · 2020-11-11T11:50:13Z

@kaihowl I tried to write a recipe on my own, but I'm stuck with my try to avoid auto generated cmake related files. See issue #3311 and my current fork state: https://github.com/andioz/conan-center-index/tree/master/recipes/pybind11. Maybe you can can find something useful.

Over all, I think it is very difficult to avoid the cmake parts for this project, many tools depend on cmake within this projects.

uilianries

We can add components later, let's update the version first.

conan-center-bot · 2020-11-13T20:56:23Z

All green in build 2 (83a2cd1294ca1e207e537593fc986f511c015ddc)! 😊

pybind11/2.4.3: Generated 1 packages. All logs here
pybind11/2.5.0: Generated 1 packages. All logs here
pybind11/2.6.0: Generated 1 packages. All logs here

prince-chrismc · 2020-11-13T22:29:20Z

We can add components later, let's update the version first.

There this #3545 duplicate

ghost · 2020-11-16T08:54:09Z

I detected other pull requests that are modifying pybind11 recipe:

[pybind11] Version update to 2.6.1 #3545

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

SSE4 requested review from danimtb and uilianries October 27, 2020 15:45

SSE4 previously approved these changes Oct 27, 2020

View reviewed changes

SpaceIm reviewed Oct 27, 2020

View reviewed changes

This was referenced Nov 10, 2020

Updatable recipes (dev) qchateau/conan-center-bot#4

Open

Updatable recipes qchateau/conan-center-bot#1

Open

[request] Updatable recipes #3470

Closed

prince-chrismc mentioned this pull request Nov 11, 2020

[request] pybind11/2.6.0 #3311

Closed

ericLemanissier mentioned this pull request Nov 12, 2020

[service] Potentially conflicting recipes #3517

Closed

Merge branch 'master' into pr/pybind11_2_6_0

fdf8160

uilianries dismissed SSE4’s stale review via fdf8160 November 13, 2020 20:48

uilianries approved these changes Nov 13, 2020

View reviewed changes

uilianries requested a review from SSE4 November 13, 2020 20:49

prince-chrismc approved these changes Nov 13, 2020

View reviewed changes

SSE4 approved these changes Nov 16, 2020

View reviewed changes

ghost mentioned this pull request Nov 16, 2020

[service] Potentially conflicting recipes #3571

Closed

ghost mentioned this pull request Nov 16, 2020

[pybind11] Version update to 2.6.1 #3545

Closed

4 tasks

conan-center-bot merged commit 7290d5b into conan-io:master Nov 16, 2020

madebr mentioned this pull request Nov 16, 2020

Cleanup for newest pybind11 (2.6.0) conanfile required? #3574

Closed

SpaceIm mentioned this pull request Jan 29, 2021

[pybind11] Error with duplicated pybind11::pybind11 target (Conan 1.33) #4407

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to pybind11 2.6.0 #3305

Update to pybind11 2.6.0 #3305

kaihowl commented Oct 24, 2020

conan-center-bot commented Oct 24, 2020

prince-chrismc commented Oct 24, 2020

prince-chrismc commented Oct 24, 2020

kaihowl commented Oct 25, 2020

prince-chrismc commented Oct 25, 2020

SpaceIm commented Oct 27, 2020

SpaceIm Oct 27, 2020

kaihowl Oct 27, 2020

Croydon Nov 15, 2020

SpaceIm Oct 27, 2020

ericriff Nov 2, 2020 •

edited

Loading

SpaceIm Nov 4, 2020 •

edited

Loading

SpaceIm Oct 27, 2020

prince-chrismc commented Nov 6, 2020

prince-chrismc commented Nov 7, 2020

andioz commented Nov 11, 2020

uilianries left a comment

conan-center-bot commented Nov 13, 2020

prince-chrismc commented Nov 13, 2020

ghost commented Nov 16, 2020

		os.rename("{}-{}".format(self.name, self.version),
		self._source_subfolder)

		self.cpp_info.build_modules = [get_path("FindPythonLibsNew.cmake"),
		get_path("pybind11Install.cmake")]

Update to pybind11 2.6.0 #3305

Update to pybind11 2.6.0 #3305

Conversation

kaihowl commented Oct 24, 2020

conan-center-bot commented Oct 24, 2020

prince-chrismc commented Oct 24, 2020

prince-chrismc commented Oct 24, 2020

kaihowl commented Oct 25, 2020

prince-chrismc commented Oct 25, 2020

SpaceIm commented Oct 27, 2020

SpaceIm Oct 27, 2020

Choose a reason for hiding this comment

kaihowl Oct 27, 2020

Choose a reason for hiding this comment

Croydon Nov 15, 2020

Choose a reason for hiding this comment

SpaceIm Oct 27, 2020

Choose a reason for hiding this comment

ericriff Nov 2, 2020 • edited Loading

Choose a reason for hiding this comment

SpaceIm Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

SpaceIm Oct 27, 2020

Choose a reason for hiding this comment

prince-chrismc commented Nov 6, 2020

prince-chrismc commented Nov 7, 2020

andioz commented Nov 11, 2020

uilianries left a comment

Choose a reason for hiding this comment

conan-center-bot commented Nov 13, 2020

prince-chrismc commented Nov 13, 2020

ghost commented Nov 16, 2020

ericriff Nov 2, 2020 •

edited

Loading

SpaceIm Nov 4, 2020 •

edited

Loading