pybind / pybind11

Seamless operability between C++11 and Python
https://pybind11.readthedocs.io/
Other
15.58k stars 2.09k forks source link

fix(cmake): pybind11.pc: use pcfiledir for relative destinations #4830

Closed mathstuf closed 10 months ago

mathstuf commented 1 year ago

If the datarootdir is absolute, just use the absolute path directly. However, if it is relative, we can compute the prefix from the location of the .pc file itself. This allows the install to be relocatable.

Description

pkgconf has odd behaviors around absolute prefix= settings. Using this makes things work there.

Suggested changelog entry:

``pybind11.pc`` is now relocatable by default as long as install destinations are not absolute paths
henryiii commented 1 year ago

I keep forgetting, @eli-schwartz, does this change sound fine?

eli-schwartz commented 1 year ago

If the datarootdir is absolute, just use the absolute path directly. However, if it is relative, we can compute the prefix [...]

Does that generally happen outside of a python wheel build? Because this currently manually sets the pcfiledir bits there from setup.py:

"-DCMAKE_INSTALL_PREFIX=pybind11",
...,
"-Dprefix_for_pc_file=${pcfiledir}/../../",

The broader issue here is that if the prefix is not absolute, then setting it as prefix=@CMAKE_INSTALL_PREFIX@ is pretty sure to be broken as pkgconf can't possibly know where that prefix is.

There's a couple issues I see with the current approach:

I wasn't generally sure of the answer to these types of questions, which is why I'd originally designed this to be absolute by default, and relocatable if you explicitly told cmake what string to relocate with.

EDIT: this seems like it is implementing a polyfill? I'm probably just bad at reading cmake scripts, my apologies.

mathstuf commented 1 year ago

The problem is if datarootdir itself is absolute, not the prefix. If destinations are all relative, the prefix is irrelevant as any location between components can be traversed via ../. If any are absolute, there's no stable way to compute a relative path, so you need to use an absolute one (unless you then take the prefix as gospel and assume non-relocatability).

checking if the datarootdir variable is absolute/relative is wrong, it will nearly always be relative for reasons totally unrelated to relocatable installs -- you want the FULL_ version which is calculated relative to the install prefix.

I was trying to keep it as compatible as possible. If erroring on an absolute CMAKE_INSTALL_DATAROOTDIR is preferable (as VTK does), this can be simpler.

this will be broken if the datarootdir has a different nesting level than "exactly one" (what is this based on? The default absolute paths use a nesting level of two for example). Hardcoding a single path traversal upwards, is as fragile as whatever else you're proposing to avoid here. Instead, this should use cmake_path(RELATIVE_PATH or implement a polyfill such as the join_paths polyfill I contributed in my initial pc file implementation.

The while loop counts the number of ../ components required.