fix: simplified pkgconfig generation #259
Conversation
|
Hello @bobvanderlinden, Thanks for the improvement,
I'm waiting ;) |
|
@alexshpilkin did the fixes, I just got the pr merged :) |
|
@bobvanderlinden Can we merge this PR without break anything on nix side and cmake? |
|
The workaround that was introduced in NixOS/nixpkgs@3bf5a3c shouldn't break any new version. Whenever the new version is released the workaround may be removed, as it won't do anything. Looks to me that everything should be ok. I just don't know the best practices for CMake + pkgconfig. I just know the change in this PR avoids workarounds in Nixpkgs and seems to align what I'm seeing in other CMake+pkgconfig projects 😅 |
|
@bobvanderlinden the PR is merged, many thanks for your contribution[s] 🚀 |
| prefix=@prefix@ | ||
| exec_prefix=@exec_prefix@ | ||
| libdir=@libdir@ | ||
| includedir=@includedir@ | ||
| prefix=@CMAKE_INSTALL_PREFIX@ | ||
| exec_prefix="${prefix}" | ||
| libdir="${exec_prefix}/lib" | ||
| includedir="${prefix}/include" |
There was a problem hiding this comment.
This is broken and breaks cmake.
(It also breaks autotools which uses the same template, but that's a side point).
Why does it break cmake? Good question. The answer is because it completely disrespects -DCMAKE_INSTALL_LIBDIR=lib64 or -DCMAKE_INSTALL_LIBDIR=lib/x86_64-linux-gnu. It assumes that:
- every path is relative to CMAKE_INSTALL_PREFIX
- every path is a hardcoded string, either "lib" or "include".
But AFAICT, the previous version of this code already handled the case where these assumptions were true, and also handled what Debian always does (lib/{triplet}), and handled systems where the libdir is "lib64".
And it handled this by including the idiomatic logic, where if CMAKE_INSTALL_* directories are absolute paths, it respects those, and if they aren't, it calculates them relative to CMAKE_INSTALL_PREFIX.
Looks to me that everything should be ok. I just don't know the best practices for CMake + pkgconfig. I just know the change in this PR avoids workarounds in Nixpkgs and seems to align what I'm seeing in other CMake+pkgconfig projects 😅
raises hand I know best practices for pkg-config, and also best practices for CMake + pkgconfig. :) This does NOT seem to align with what I see in other CMake projects that work. Usually, I and various others PR "fixes" that remove the logic you added, and add the logic you removed. :(
There was a problem hiding this comment.
@eli-schwartz thanks for the catch and investigation #272 reverts this as you suggested
| if (IS_ABSOLUTE "${CMAKE_INSTALL_INCLUDEDIR}") | ||
| set(includedir "${CMAKE_INSTALL_INCLUDEDIR}") | ||
| else() | ||
| set(includedir "\${prefix}/${CMAKE_INSTALL_INCLUDEDIR}") | ||
| endif() | ||
| if (IS_ABSOLUTE "${CMAKE_INSTALL_LIBDIR}") | ||
| set(libdir "${CMAKE_INSTALL_LIBDIR}") | ||
| else() | ||
| set(libdir "\${exec_prefix}/${CMAKE_INSTALL_LIBDIR}") | ||
| endif() |
There was a problem hiding this comment.
It's not clear to me how it went wrong in NixOS, unless IS_ABSOLUTE in cmake itself is broken? Is NixOS setting a bad value for -DCMAKE_INSTALL_LIBDIR and -DCMAKE_INSTALL_INCLUDEDIR?
If these are set to "lib" and "include" respectively, then NixOS should generate pkg-config files with the same logic both before and after this PR.
If these are set to absolute paths, then NixOS should generate absolute paths already.
I'm not even sure how it's possible to end up with:
prefix=/nix/store/3b25lbw0vnglfar91mich3mliqchxmiy-cglm-0.8.5
exec_prefix=/nix/store/3b25lbw0vnglfar91mich3mliqchxmiy-cglm-0.8.5
libdir=${prefix}//nix/store/3b25lbw0vnglfar91mich3mliqchxmiy-cglm-0.8.5/lib
includedir=${prefix}//nix/store/3b25lbw0vnglfar91mich3mliqchxmiy-cglm-0.8.5/include
This would imply that prefix= sees -DCMAKE_INSTALL_PREFIX=/nix/store/hashed-dir-cglm-0.8.5 but that libdir and includedir see CMAKE_INSTALL_PREFIX with an ending slash, and see -DCMAKE_INSTALL_LIBDIR=nix/store/hashed-dir-cglm-0.8.5/include without a leading slash? Alternatively, if (IS_ABSOLUTE "/nix/store/hashed-dir-cglm-0.8.5") reports that that path is NOT an absolute path, which I simply do not understand.
@jtojnar is both a Nix person and the author of https://github.com/jtojnar/cmake-snips, which is a cmake function library that implements effectively the same logic as this PR moves away from. The README for the snip basically says "people frequently break Nix by not using this snip" :) so I think that NixOS probably specifically wants to avoid this PR after all?
But maybe @jtojnar can help shed light on why the NixOS package for cglm was getting weird pkg-config files.
There was a problem hiding this comment.
these are the cmake flags for cglm
cglm> cmake flags:
-DCMAKE_FIND_USE_SYSTEM_PACKAGE_REGISTRY=OFF -DCMAKE_FIND_USE_PACKAGE_REGISTRY=OFF
-DCMAKE_EXPORT_NO_PACKAGE_REGISTRY=ON
-DCMAKE_BUILD_TYPE=Release
-DBUILD_TESTING=OFF
-DCMAKE_INSTALL_LOCALEDIR=/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/share/locale
-DCMAKE_INSTALL_LIBEXECDIR=/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/libexec
-DCMAKE_INSTALL_LIBDIR=/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/lib
-DCMAKE_INSTALL_DOCDIR=/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/share/doc/cglm
-DCMAKE_INSTALL_INFODIR=/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/share/info
-DCMAKE_INSTALL_MANDIR=/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/share/man
-DCMAKE_INSTALL_OLDINCLUDEDIR=/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/include
-DCMAKE_INSTALL_INCLUDEDIR=/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/include
-DCMAKE_INSTALL_SBINDIR=/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/sbin
-DCMAKE_INSTALL_BINDIR=/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/bin
-DCMAKE_INSTALL_NAME_DIR=/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/lib
-DCMAKE_POLICY_DEFAULT_CMP0025=NEW
-DCMAKE_OSX_SYSROOT=
-DCMAKE_FIND_FRAMEWORK=LAST
-DCMAKE_STRIP=/nix/store/wn31i3dzwahz6ccws8bs1nwyqrpgsvj7-gcc-wrapper-11.3.0/bin/strip
-DCMAKE_RANLIB=/nix/store/wn31i3dzwahz6ccws8bs1nwyqrpgsvj7-gcc-wrapper-11.3.0/bin/ranlib
-DCMAKE_AR=/nix/store/wn31i3dzwahz6ccws8bs1nwyqrpgsvj7-gcc-wrapper-11.3.0/bin/ar
-DCMAKE_C_COMPILER=gcc
-DCMAKE_CXX_COMPILER=g++
-DCMAKE_INSTALL_PREFIX=/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5
and yet they fail on 0.8.5 which doesn't have this commit
There was a problem hiding this comment.
Alternatively,
if (IS_ABSOLUTE "/nix/store/hashed-dir-cglm-0.8.5")reports that that path is NOT an absolute path, which I simply do not understand.
So this is expanding out to:
if (IS_ABSOLUTE "/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/include")
set(includedir "/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/include")
else()
set(includedir "\${prefix}//nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/include ")
endif()
if (IS_ABSOLUTE "/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/lib")
set(libdir "/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/lib")
else()
set(libdir "\${exec_prefix}//nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/lib")
endif()
Why is cmake claiming IS_ABSOLUTE is not, in fact, absolute, and then following all the "else" branches here? This doesn't make sense -- it directly contradicts CMake's documentation.
There was a problem hiding this comment.
Here's the cmake source for FileIsFullPath https://github.com/Kitware/CMake/blob/02599da236fd22db0dcfb6503194e5bad086aea9/Source/kwsys/SystemTools.cxx#L4262-L4293
are the quotes messing with it? also note that the directory does not exist at that point in the build
There was a problem hiding this comment.
The quotes really should not matter, that's just cmake's optional syntax for strings.
Directories shouldn't matter either. I'm not using Nix.
Test file:
project(test LANGUAGES C)
if (IS_ABSOLUTE "/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/include")
message("literal is absolute")
else()
message("literal is not absolute")
endif()
if (IS_ABSOLUTE "${CMAKE_INSTALL_INCLUDEDIR}")
message("includedir variable as '${CMAKE_INSTALL_INCLUDEDIR}' is absolute")
else()
message("includedir variable as '${CMAKE_INSTALL_INCLUDEDIR}' is not absolute")
endif()$ cmake . -DCMAKE_INSTALL_INCLUDEDIR=/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/include
literal is absolute
includedir variable as '/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/include' is absolute
-- Configuring done
-- Generating done
-- Build files have been written to: /tmp/cmtest
There was a problem hiding this comment.
Thank you for your investigation!
your test works for me too in nix develop
and the pc in /build contains the correct paths too in nix develop after running cmakeConfigurePhase
prefix=/home/artturin/cglm/outputs/out
exec_prefix=/home/artturin/cglm/outputs/out
libdir=/home/artturin/cglm/outputs/out/lib
includedir=/home/artturin/cglm/outputs/out/include
i made this nix reproducer which weirdly works
let
pkgs = ((builtins.getFlake (toString ./.)).legacyPackages.${builtins.currentSystem});
cmakefile = pkgs.writeText "CMakeLists.txt" ''
project(test LANGUAGES C)
if (IS_ABSOLUTE "/nix/store/rgfhd3jk5yflwcy112jwqvnx0dfw6bsr-cglm-0.8.5/include")
message("literal is absolute")
else()
message("literal is not absolute")
endif()
if (IS_ABSOLUTE "''${CMAKE_INSTALL_INCLUDEDIR}")
message("includedir variable as ' ''${CMAKE_INSTALL_INCLUDEDIR}' is absolute")
else()
message("includedir variable as ' ''${CMAKE_INSTALL_INCLUDEDIR}' is not absolute")
endif()
set(prefix ''${CMAKE_INSTALL_PREFIX})
set(exec_prefix ''${CMAKE_INSTALL_PREFIX})
if (IS_ABSOLUTE "''${CMAKE_INSTALL_INCLUDEDIR}")
set(includedir "''${CMAKE_INSTALL_INCLUDEDIR}")
else()
set(includedir "\''${prefix}/''${CMAKE_INSTALL_INCLUDEDIR}")
endif()
if (IS_ABSOLUTE "''${CMAKE_INSTALL_LIBDIR}")
set(libdir "''${CMAKE_INSTALL_LIBDIR}")
else()
set(libdir "\''${exec_prefix}/''${CMAKE_INSTALL_LIBDIR}")
endif()
configure_file(''${CMAKE_CURRENT_LIST_DIR}/test.pc.in ''${CMAKE_CURRENT_LIST_DIR}/test.pc @ONLY)
'';
pcfile = pkgs.writeText "test.pc.in" ''
prefix=@prefix@
exec_prefix=@exec_prefix@
libdir=@libdir@
includedir=@includedir@
'';
in
pkgs.stdenv.mkDerivation {
name = "cmake-test";
nativeBuildInputs = [ pkgs.cmake ];
buildCommand = ''
cp ${cmakefile} CMakeLists.txt
cp ${pcfile} test.pc.in
cmakeConfigurePhase
mkdir $out
mv /build/* $out
'';
}
There was a problem hiding this comment.
0.8.5 did not have the is_absolute check:
https://github.com/recp/cglm/blob/v0.8.5/CMakeLists.txt#L153-L154
So this PR breaks custom directory names and removes what would already work correctly for Nixpkgs.
There was a problem hiding this comment.
@eli-schwartz Thanks for bringing this up. This has been a while back and I can't really remember why I did this PR, but I clearly wasn't aware of the earlier fixes. I think I went overboard simplifying the cmake configuration. I need to learn more about cmake best practices. It has confused me in another project as well. If you have any sources on how to write proper cmake configuration I'm very interested.
Sorry for the confusion/problems this has caused. The revert seems like the best solution for now.
| prefix=@CMAKE_INSTALL_PREFIX@ | ||
| exec_prefix="${prefix}" | ||
| libdir="${exec_prefix}/lib" | ||
| includedir="${prefix}/include" |
There was a problem hiding this comment.
| prefix=@CMAKE_INSTALL_PREFIX@ | |
| exec_prefix="${prefix}" | |
| libdir="${exec_prefix}/lib" | |
| includedir="${prefix}/include" | |
| prefix=@CMAKE_INSTALL_FULL_PREFIX@ | |
| exec_prefix="${prefix}" | |
| libdir=@CMAKE_INSTALL_FULL_LIBDIR@ | |
| includedir=@CMAKE_INSTALL_FULL_INCLUDEDIR@ |
There was a problem hiding this comment.
That results in non-idiomatic pkg-config files that don't handle relocation.
The variables in pkg-config files are expected to contain interpolations (e.g.
completiondir=${datadir}/bash-completion) so that projects can install files relative their own directories instead of the ones of the project whose variables they use (see https://www.bassi.io/articles/2018/03/15/pkg-config-and-paths/). This means you cannot just use theCMAKE_INSTALL_FULL_<dir>variables and need to construct the paths yourself.
Currently the packages cglm on NixOS has incorrect paths in
cglm.pc:There have been made 2 fixes in parallel in NixOS (NixOS/nixpkgs#181875 and NixOS/nixpkgs#190982). It seemed best to fix this upstream by aligning the
cglm.pc.inmore with other CMake packages.The output now resembles the output in most packages and aligns with the examples I found online (https://www.scivision.dev/cmake-generate-pkg-config/).
Alternatively
CMAKE_INSTALL_FULL_*DIRcould be used.I'll ask @Artturin if he could have a look at this PR before asking for a review from any of the maintainers of cglm.