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

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

Merged
merged 4 commits into from
Nov 16, 2023

Conversation

mathstuf
Copy link
Contributor

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

@mathstuf mathstuf requested a review from henryiii as a code owner August 31, 2023 11:10
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.
@henryiii
Copy link
Collaborator

henryiii commented Sep 15, 2023

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

CMakeLists.txt Outdated Show resolved Hide resolved
@eli-schwartz
Copy link
Contributor

eli-schwartz commented Sep 15, 2023

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:

  • 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.
  • 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.

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
Copy link
Contributor Author

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.

@henryiii henryiii merged commit dc9b395 into pybind:master Nov 16, 2023
84 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Nov 16, 2023
@henryiii henryiii changed the title pybind11.pc: use pcfiledir for relative destinations fix(cmake): pybind11.pc: use pcfiledir for relative destinations Nov 16, 2023
@mathstuf mathstuf deleted the pc-use-pcfiledir branch November 16, 2023 16:50
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Mar 27, 2024
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.

3 participants