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

add installation support for pkg-config dependency detection #4077

Merged
merged 2 commits into from Aug 9, 2022

Conversation

eli-schwartz
Copy link
Contributor

@eli-schwartz eli-schwartz commented Jul 17, 2022

pkg-config is a build-system agnostic alternative to pybind11Config.cmake that can be used from build systems other than cmake.

Fixes #230

Suggested changelog entry:

* Include a pkg-config file when installing pybind11, such as in the Python package.

endforeach()
set(${joined_path} "${temp_path}" PARENT_SCOPE)
set(temp_path "${first_path_segment}")
foreach(current_segment IN LISTS ARGN)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there some way to tell pre-commit.ci that this file belongs to another repo and shouldn't be autoformatted as that will result in a confusing file delta between here and the upstream version? 👀

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, you can add an exclude key regex to the pre-commit-config.yaml for the relevant hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filename seems to have a dot in front of it...

This file is very big and I have basically zero knowledge of how it works other than "frustrating thing that messes up my commit messages", can I have a small hint at what I'm looking at, here? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

@henryiii henryiii Jul 17, 2022

Choose a reason for hiding this comment

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

Also could be worth mentioning this file can be removed once we depend on CMake 3.20+.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks. Global exclude sounds helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added comments in CMakeLists.txt, marked with # TODO: for easy searchability.

@henryiii
Copy link
Collaborator

I'm traveling & at conferences, I'd like to take a look at the generated file. I might want to tweak the CMake code just a bit too. I'll have a bit more time starting Wednesday, so ping me ~Thursday if I don't get back to this by then. Thanks!

@eli-schwartz
Copy link
Contributor Author

Ping. :)

I'd like to take a look at the generated file.

BTW, here is the generated code for the regular wheel:

$ bsdtar -xOf dist/pybind11-2.11.0.dev1-py3-none-any.whl pybind11/share/pkgconfig/pybind11.pc
prefix=${pcfiledir}/../../
includedir=${prefix}/include

Name: pybind11
Description: Seamless operability between C++11 and Python
Version: 2.11.0
Cflags: -I${includedir}

It's the same content for the pybind11_global wheel except, of course, the location.
File lists, more or less:

pybind11:

pybind11/share/cmake/pybind11/FindPythonLibsNew.cmake
pybind11/share/cmake/pybind11/pybind11Common.cmake
pybind11/share/cmake/pybind11/pybind11Config.cmake
pybind11/share/cmake/pybind11/pybind11ConfigVersion.cmake
pybind11/share/cmake/pybind11/pybind11NewTools.cmake
pybind11/share/cmake/pybind11/pybind11Targets.cmake
pybind11/share/cmake/pybind11/pybind11Tools.cmake
pybind11/share/pkgconfig/pybind11.pc

pybind11_global

pybind11_global-2.11.0.dev1.data/data/share/cmake/pybind11/FindPythonLibsNew.cmake
pybind11_global-2.11.0.dev1.data/data/share/cmake/pybind11/pybind11Common.cmake
pybind11_global-2.11.0.dev1.data/data/share/cmake/pybind11/pybind11Config.cmake
pybind11_global-2.11.0.dev1.data/data/share/cmake/pybind11/pybind11ConfigVersion.cmake
pybind11_global-2.11.0.dev1.data/data/share/cmake/pybind11/pybind11NewTools.cmake
pybind11_global-2.11.0.dev1.data/data/share/cmake/pybind11/pybind11Targets.cmake
pybind11_global-2.11.0.dev1.data/data/share/cmake/pybind11/pybind11Tools.cmake
pybind11_global-2.11.0.dev1.data/data/share/pkgconfig/pybind11.pc

@eli-schwartz
Copy link
Contributor Author

@henryiii ping

@henryiii
Copy link
Collaborator

Thanks! I'm back in Princeton now after nearly a month of travel.

Is there any reason it's joining the path rather than hardcoding the separator, as in ${pcfiledir}/../../? The generated file gets including in the wheel, and must be usable from all OS's (and ideally creatable on all OSs identically), so ${pcfiledir}\..\..\ would be invalid.

@eli-schwartz
Copy link
Contributor Author

The JoinPaths function uses forward slashes anyway, exactly for portability reasons. And the paths need to be joined because cmake's includedir may or may not be an absolute path. (It will be a relative path when invoking cmake from setup.py, though.)

@eli-schwartz
Copy link
Contributor Author

eli-schwartz commented Aug 1, 2022

@henryiii the test that you added is failing due to platform differences between \r\n and \n. Not sure what to do about that, I do notice that the pybind11Config.cmake check only tries to assert that a set(...) exists in the file, it's very newline style agnostic.

Is this another of those "setuptools re-encodes all the newlines" issues? :(

@eli-schwartz
Copy link
Contributor Author

By the way, merging this PR would help Void Linux successfully cross-compile SciPy. See scipy/scipy#16783

@henryiii
Copy link
Collaborator

henryiii commented Aug 5, 2022

Sorry, in the third of four conferences in a row. And preparation for CPython 3.10rc1 is taking free time too. This just needs to address the failing test. Not too worried about the line endings, happy to normalize them in the test. I hopefully can do it by Monday, ping me if I don’t! (Or you can do it if you want)

@eli-schwartz
Copy link
Contributor Author

No problem, take your time. I won't have time before then either, anyway. :)

eli-schwartz and others added 2 commits August 8, 2022 15:40
pkg-config is a buildsystem-agnostic alternative to
`pybind11Config.cmake` that can be used from build systems other than
cmake.

Fixes pybind#230
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@rwgk
Copy link
Collaborator

rwgk commented Aug 8, 2022

This is totally over my head but I looked out of curiosity. Is this impression correct?

  • pkg-config is build-system agnostic, but the implementation of the pybind11 pkg-config support relies on cmake?

If that's true, it might be nice to add that as a note to the PR description.

Initially the phrase "alternative to pybind11Config.cmake" in the PR description led me to believe the pkg-config support will not involve cmake at all.

@eli-schwartz
Copy link
Contributor Author

CMake is the build system that pybind11 uses, so CMake is used to generate the pkg-config file. Once generated (for example by producing a python wheel, or by running cmake && make install in a Linux distro) the resultant installable files are agnostic.

For the purposes of e.g. SciPy (and more generally meson) we don't involve CMake.

@rwgk
Copy link
Collaborator

rwgk commented Aug 8, 2022

Got it, thanks for the explanation!

@henryiii henryiii merged commit 5bdd3d5 into pybind:master Aug 9, 2022
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Aug 9, 2022
@eli-schwartz eli-schwartz deleted the pkgconfig branch August 10, 2022 02:59
@eli-schwartz
Copy link
Contributor Author

@henryiii thanks for figuring out the CI. Really happy to see this landing. :)

@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Oct 20, 2022
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.

Add pkg-config support
4 participants