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 Nix pkg-config support #374

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

drawbu
Copy link

@drawbu drawbu commented Oct 13, 2024

@ChrisThrasher
Copy link
Member

Why not match what SFML does? SFML retains ${exec_prefix}/. I'm only loosely familiar with this situation but isn't Nix weird about treating paths in a way that no other OS does, hence these kinds of issues?

@drawbu
Copy link
Author

drawbu commented Oct 13, 2024

Hello !
Sorry, that pr was in my draft for a long time. I apologize for not providing a lot of context around it...

I actually tested with both libdir=@CMAKE_INSTALL_FULL_LIBDIR@ and libdir=${exec_prefix}/@SFML_RELATIVE_INSTALL_LIBDIR@ (like in the SFML pr)

Both worked fine (on my end) when I tested, but do you prefer the second option?

@ChrisThrasher
Copy link
Member

I prefer the option which is most in line with SFML and requires the least possible changes to CSFML.

@drawbu
Copy link
Author

drawbu commented Oct 13, 2024

From what I understand, CMAKE_INSTALL_LIBDIR is not really precise in content, so using CMAKE_INSTALL_FULL_LIBDIR and CMAKE_INSTALL_RELATIVE_LIBDIR are prefered.

And for the nix issue, it appears that CMAKE_INSTALL_LIBDIR is set to be the full path, so appending it to ${exec_prefix}/ create weird path

@drawbu
Copy link
Author

drawbu commented Oct 13, 2024

Okay I'll change that :)

@@ -1,6 +1,6 @@
prefix=@CMAKE_INSTALL_PREFIX@
exec_prefix=${prefix}
libdir=${exec_prefix}/@CMAKE_INSTALL_LIBDIR@
libdir=${exec_prefix}/@SFML_RELATIVE_INSTALL_LIBDIR@
Copy link
Member

Choose a reason for hiding this comment

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

SFML_RELATIVE_INSTALL_LIBDIR doesn't exist in CSFML. Go look at that PR you linked to see where I defined that new variable and do something similar with CSFML.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah just saw that my bad :(

Copy link
Author

Choose a reason for hiding this comment

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

What do you think of this?
It's heavily inspired by what was done on the SFML side

@texus
Copy link
Contributor

texus commented Oct 13, 2024

The code shouldn't blindly be based on what SFML is using, because that implementation is also partially broken (SFML/SFML#2917).

I would expect that ${exec_prefix}/@CSFML_PKGCONFIG_INSTALL_DIR@ would be the correct path when CSFML_PKGCONFIG_INSTALL_DIR is a relative path, but that is probably wrong again when CSFML_PKGCONFIG_INSTALL_DIR is set to an absolute path.
Nevermind, libdir needs to be the path to the libraries and not to the pkg-files.

@texus
Copy link
Contributor

texus commented Oct 13, 2024

I can confirm that this PR solves the issue of libdir being wrong when an absolute path is set in CMAKE_INSTALL_LIBDIR. However I'm not certain if it is the best solution, and I have found an unrelated issue with the pkgconfig generation during testing.

The following is generated inside the .pc files when calling cmake with -DCMAKE_INSTALL_LIBDIR=/home/texus/some_dir:

  • Without this PR:
    libdir=${exec_prefix}//home/texus/some_dir

  • With this PR:
    libdir=${exec_prefix}/../../home/texus/some_dir

  • When using libdir=@CMAKE_INSTALL_FULL_LIBDIR@ instead of libdir=${exec_prefix}/@CSFML_RELATIVE_INSTALL_LIBDIR@:
    libdir=/home/texus/some_dir

So I actually like the CMAKE_INSTALL_FULL_LIBDIR option better: it doesn't require an additional variable. The file(RELATIVE_PATH ...) call in cmake/Config.cmake isn't needed with this method.


There is also an unrelated issue: when the prefix that is passed during installation is different from when configuring, the prefix variable in the .pc file will still point to the old location. So when running the following commands, the paths in the .pc files will point to /usr/local instead of to the whatever folder. This is because in the second command the CMakeLists.txt does not seem to be executed again, so configure_file is never called with the new paths.

cmake --build build
cmake --install build --prefix whatever

The same issue also exists when using CMAKE_INSTALL_FULL_LIBDIR instead of relying on the prefix variable. This is even documented in the CMAKE_INSTALL_FULL_<dir> documentation:

they do not work with the cmake --install command's --prefix option

Edit: This seems to be an issue with only hacky solutions (see https://discourse.cmake.org/t/how-to-generate-pc-pkg-config-file-supporting-prefix-of-the-cmake-install/4109 and https://gitlab.kitware.com/cmake/cmake/-/issues/24887), so maybe we can just ignore it and assume that people who want pkg-config files will set CMAKE_INSTALL_PREFIX in a normal way instead of setting a different prefix at install time.

@drawbu
Copy link
Author

drawbu commented Oct 14, 2024

@ChrisThrasher what's your opinion regarding this @CMAKE_INSTALL_FULL_LIBDIR@ vs ${exec_prefix}/@CSFML_RELATIVE_INSTALL_LIBDIR@ situation ?

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