Pybind11 fix linkage #13283

planetmarshall · 2022-10-03T15:05:43Z

Specify library name and version: pybind11

Update to Conan V2 and new toolchains
Attempt to fix issue raised in [pybind11] Do not support Mac for now #11075

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.

planetmarshall · 2022-10-03T15:15:57Z

Something is causing pybind to link in libpython which it shouldn't be doing for a module. A modification to the test package can workaround the issue, but I'd still like to get to the bottom of it as it will break consumers using the typical path.

# pybind11 method:
pybind11_add_module(MyModule1 src1.cpp)

Results in a linking error because libpython is linked in

# Python method:
Python_add_library(MyModule2 src2.cpp)
target_link_libraries(MyModule2 pybind11::headers)

However, links correctly.

It's a bit mysterious as conan targets only links the headers, they don't do anything else. Linking in Python (or not) is handled by pbind11's custom CMake scripts.

See Pybind11 CMake helpers

ghost · 2022-10-03T15:20:56Z

I detected other pull requests that are modifying pybind11/all recipe:

pybind11 cmakedeps #9348

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

planetmarshall · 2022-10-03T15:28:12Z

recipes/pybind11/all/test_package/conanfile.py

+        if can_run(self):
+            python_path = os.path.join(self.build_folder, self.cpp.build.libdirs[0])
+            module_path = os.path.join(self.source_folder, "test.py")
+            self.run(f"PYTHONPATH={python_path} {self._python_interpreter} {module_path}", env="conanrun")


Is there a better way to set the PYTHONPATH env var in Conan V2? I can't set it in generate as I don't have access to self.build_folder at that point.

That's a great question. Hopefully someone who knows will update us.

jwillikers · 2022-10-04T13:44:49Z

recipes/pybind11/all/test_package/CMakeLists.txt

 find_package(pybind11 REQUIRED CONFIG)
-
-pybind11_add_module(test_package MODULE test_package.cpp)
+#pybind11_add_module(test_package MODULE test_package.cpp)


Lingering comment?

PR still in draft - there are still a few things I want to test.

ericLemanissier · 2022-10-14T08:02:01Z

@planetmarshall please close the PR for 10 second then reopen it so that it retriggers a build and finally merge this change

uilianries · 2022-10-14T15:21:10Z

My guess is, it failed with a runtime error. Need to run on Windows to check it.

conan-center-bot · 2022-10-14T17:29:42Z

All green in build 12 (430971e3a835eb0c8dd7026e8b7df021c018025b):

pybind11/2.7.1@:
All packages built successfully! (All logs)
pybind11/2.8.1@:
All packages built successfully! (All logs)
pybind11/2.9.1@:
All packages built successfully! (All logs)
pybind11/2.10.0@:
All packages built successfully! (All logs)
pybind11/2.9.2@:
All packages built successfully! (All logs)

Whilst attempting to write a test executable with an embedded Python interpreter for OpenAssetIO#739, it turned out that the old problem reported in OpenAssetIO#528 was still present. To recap, ultimately (via circuitous logic) every CMake target created by the Conan pybind11 package links to `libpython` (the `pybind11::embed` target is the only one that should). This means that Python extension modules (`pybind11::module`s) link to `libpython`, causing linker errors at runtime or even segfaults due to ODR violations. As of conan-io/conan-center-index#13283 this bug was fixed upstream. However, we still see it because the bug was not fixed for the deprecated `cmake_find_package` generator, which we use. It has not affected us so far because we haven't added in the `Development.Embed` component to our `find_package(Python` call. As soon as we add that component (e.g. for adding the aforementioned Python unit test) we see this problem. So modernise our `conanfile.py` to use the non-deprecated `CMakeDeps` generator. `CMakeDeps` insists on having the `build_type` setting available, so add that. Unfortunately `CMakeDeps` is designed to install dependencies with the same configuration (Release, Debug, etc) as the parent project, which is usually undesirable - we want Release versions of dependencies whilst we develop the project in Debug mode. This is logged upstream in conan-io/conan#11607, where a workaround is posted, which is used here (see code comments). Signed-off-by: David Feltell <david.feltell@foundry.com>

planetmarshall added 3 commits October 3, 2022 14:07

[pybind11] update to new toolchain

161912f

remove old cmakelists

790bea7

[pybind11] update test packages

c8a0945

planetmarshall marked this pull request as draft October 3, 2022 15:06

planetmarshall mentioned this pull request Oct 3, 2022

[service] Failures in linter affecting wrong recipe. #13284

Closed

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label Oct 3, 2022

[pybind11] fix missing source in v1 test package

3e3957b

conan-center-bot removed the Failed label Oct 3, 2022

ghost mentioned this pull request Oct 3, 2022

[service] Potentially conflicting recipes #3571

Closed

planetmarshall commented Oct 3, 2022

View reviewed changes

planetmarshall mentioned this pull request Oct 3, 2022

[package] pybind11 ignores targets and always link against the whole list of components #6605

Open

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label Oct 3, 2022

[pybind11] use legacy env tools in test package

a8f845d

conan-center-bot removed the Failed label Oct 3, 2022

planetmarshall mentioned this pull request Oct 3, 2022

[BUG] Fatal Python error: PyMUTEX_LOCK(gil->mutex) failed pybind/pybind11#3081

Open

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label Oct 3, 2022

[pybind] remove win_bash requirement from test package

e2014a3

conan-center-bot removed the Failed label Oct 3, 2022

This comment has been minimized.

Sign in to view

jwillikers previously approved these changes Oct 4, 2022

View reviewed changes

planetmarshall added 3 commits October 8, 2022 23:45

[pybind11] fix component linkage order

38e1720

[pybind11] remove reference to unsupported versions

4d0b54c

[pybind11] fix include dir

3bc0175

planetmarshall dismissed jwillikers’s stale review via 3bc0175 October 8, 2022 23:02

planetmarshall marked this pull request as ready for review October 8, 2022 23:03

conan-center-bot added the Service Under Maintenance label Oct 11, 2022

This comment has been minimized.

Sign in to view

[pybind11] fix typo

13b4cd9

This comment has been minimized.

Sign in to view

jwillikers previously approved these changes Oct 11, 2022

View reviewed changes

prince-chrismc previously approved these changes Oct 11, 2022

View reviewed changes

conan-center-bot removed the Service Under Maintenance label Oct 11, 2022

uilianries previously approved these changes Oct 13, 2022

View reviewed changes

planetmarshall closed this Oct 14, 2022

planetmarshall reopened this Oct 14, 2022

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label Oct 14, 2022

[pybind11] ensure translation of win32 path in cmake toolchain file

430971e

planetmarshall dismissed stale reviews from uilianries, prince-chrismc, and jwillikers via 430971e October 14, 2022 14:14

conan-center-bot removed the Failed label Oct 14, 2022

jwillikers approved these changes Oct 14, 2022

View reviewed changes

uilianries approved these changes Oct 14, 2022

View reviewed changes

conan-center-bot merged commit 1c28d5c into conan-io:master Oct 14, 2022

prince-chrismc mentioned this pull request Oct 17, 2022

Conan 2.0 Migration Effort #12888

Closed

planetmarshall deleted the pybind11-fix-linkage branch March 4, 2024 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pybind11 fix linkage #13283

Pybind11 fix linkage #13283

planetmarshall commented Oct 3, 2022

This comment has been minimized.

planetmarshall commented Oct 3, 2022

ghost commented Oct 3, 2022

planetmarshall Oct 3, 2022 •

edited

Loading

jwillikers Oct 4, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jwillikers Oct 4, 2022

planetmarshall Oct 4, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ericLemanissier commented Oct 14, 2022

This comment has been minimized.

uilianries commented Oct 14, 2022

conan-center-bot commented Oct 14, 2022

Pybind11 fix linkage #13283

Pybind11 fix linkage #13283

Conversation

planetmarshall commented Oct 3, 2022

This comment has been minimized.

planetmarshall commented Oct 3, 2022

ghost commented Oct 3, 2022

planetmarshall Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

jwillikers Oct 4, 2022

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jwillikers Oct 4, 2022

Choose a reason for hiding this comment

planetmarshall Oct 4, 2022

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ericLemanissier commented Oct 14, 2022

This comment has been minimized.

uilianries commented Oct 14, 2022

conan-center-bot commented Oct 14, 2022

planetmarshall Oct 3, 2022 •

edited

Loading