Closed Sigmanificient closed 4 months ago
Thanks for this bug report. Checking the cmake documentation seems to indicate we should indeed use "CMAKE_INSTALL_FULL_LIBDIR/INCLUDEDIR" instead. Any objections @vt-alt ?
Well, I don't know anything about Nix. That path looks bizarre:
2:libdir=${prefix}//nix/store/m6lrn85hylxmnhz1zhsy6sibq0jnxlnb-liboqs-0.10.0/lib
In ALT we have correct paths in final liboqs.pc
;
prefix=/usr
libdir=${prefix}/lib64
includedir=${prefix}/include
(Using ${prefix}
is common practice.) I tested with proposed changes
-libdir=${prefix}/@CMAKE_INSTALL_LIBDIR@
-includedir=${prefix}/@CMAKE_INSTALL_INCLUDEDIR@
+libdir=@CMAKE_INSTALL_FULL_LIBDIR@
+includedir=@CMAKE_INSTALL_FULL_INCLUDEDIR@
And it produced correct paths either:
prefix=/usr
libdir=/usr/lib64
includedir=/usr/include
So I don't object this improvement. But who knows what it will produce on Nix. Documentation says
an absolute path is constructed typically by prepending the value of the CMAKE_INSTALL_PREFIX variable
So there are chances it will produce just the same value.
There are also chances this is just Nix packaging bug which we should not try to "fix".
Also, the docs say:
LIBDIR object code libraries (lib or lib64)
On Debian, this may be lib/
when CMAKE_INSTALL_PREFIX is /usr.
So this should be lib
, lib64
, or lib/<multiarch-tuple>
on Debian and not something like /nix/store/m6lrn85hylxmnhz1zhsy6sibq0jnxlnb-liboqs-0.10.0/lib
.
By this I conclude Nix [packager] maybe misdefining it.
I checked on Debian 12 in Docker:
root@e3e602170cd4:/# cat /usr/lib/x86_64-linux-gnu/pkgconfig/liboqs.pc
prefix=/usr
libdir=${prefix}/lib
includedir=${prefix}/include
...
In Fedora Rawhide:
[root@b309a2d23cc7 /]# cat /usr/lib64/pkgconfig/liboqs.pc
prefix=/usr
libdir=${prefix}/lib64
includedir=${prefix}/include
...
All expected correct values.
So my thought is, while I don't oppose the change, that the values are already perfectly correct, and the change will not change much, and, perhaps, don't fix Nix packaging.
Thanks for the analysis, @vt-alt . Based on this I tend to agree we leave things as-is.
By this I conclude Nix [packager] maybe misdefining it.
Indeed, the NIX path appears strange: It seems it causes a full path to be appended to "${prefix}": Can you check (why/whether) this so, @Sigmanificient ? Could you check whether the change suggested by the cmake
documentation would indeed (or not) improve the situation for you?
@vt-alt Using the following patch:
--- a/src/liboqs.pc.in 2024-05-09 14:54:04.989767336 +0200
+++ b/src/liboqs.pc.in 2024-05-09 14:54:50.361324577 +0200
@@ -1,6 +1,6 @@
prefix=@CMAKE_INSTALL_PREFIX@
-libdir=${prefix}/@CMAKE_INSTALL_LIBDIR@
-includedir=${prefix}/@CMAKE_INSTALL_INCLUDEDIR@
+libdir=@CMAKE_INSTALL_FULL_LIBDIR@
+includedir=@CMAKE_INSTALL_FULL_INCLUDEDIR@
Name: @PROJECT_NAME@
Description: Library for quantum-safe cryptographic algorithms
The package build successfully and the generated pc
file is also seems to be correct:
$ nix-build -A liboqs.dev
/nix/store/944qsiqygszzvj2s98swyl9bl2k7mvlb-liboqs-0.10.0-dev
$ cat result-dev/lib/pkgconfig/liboqs.pc
prefix=/nix/store/86dhaq2f3kh3fy5ck8szj7xl68lhagii-liboqs-0.10.0
libdir=/nix/store/86dhaq2f3kh3fy5ck8szj7xl68lhagii-liboqs-0.10.0/lib
includedir=/nix/store/944qsiqygszzvj2s98swyl9bl2k7mvlb-liboqs-0.10.0-dev/include
Name: liboqs
Description: Library for quantum-safe cryptographic algorithms
Version: 0.10.0
Requires.private: openssl
Cflags: -I${includedir}
Libs: -L${libdir} -loqs
@Sigmanificient Do you redefine CMAKE_INSTALL_LIBDIR
somehow while building liboqs? Looks like it already contains prefix
which seems incorrect (see the docs). It should contain only lib
for your case.
Also, can you show your full cmake
command? it's hard to guess what went wrong without this or verbose build log.
I use the attributes used in the derivation:
patches = [
./fix-openssl-detection.patch
# liboqs.pc.in path were modified in this commit
# causing malformed path with double slashes.
./fix-pc-file-variables.patch
];
nativeBuildInputs = [ cmake ninja ];
buildInputs = [ openssl ];
cmakeFlags = [
"-DBUILD_SHARED_LIBS=${if enableStatic then "OFF" else "ON"}"
"-DOQS_DIST_BUILD=ON"
"-DOQS_BUILD_ONLY_LIB=ON"
];
dontFixCmake = true; # fix CMake file will give an error
outputs = [ "out" "dev" ];
Nix logs report the following cmake flags (I've put each on their own line):
-GNinja
-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/86dhaq2f3kh3fy5ck8szj7xl68lhagii-liboqs-0.10.0/share/locale
-DCMAKE_INSTALL_LIBEXECDIR=/nix/store/86dhaq2f3kh3fy5ck8szj7xl68lhagii-liboqs-0.10.0/libexec
-DCMAKE_INSTALL_LIBDIR=/nix/store/86dhaq2f3kh3fy5ck8szj7xl68lhagii-liboqs-0.10.0/lib
-DCMAKE_INSTALL_DOCDIR=/nix/store/86dhaq2f3kh3fy5ck8szj7xl68lhagii-liboqs-0.10.0/share/doc/liboqs
-DCMAKE_INSTALL_INFODIR=/nix/store/86dhaq2f3kh3fy5ck8szj7xl68lhagii-liboqs-0.10.0/share/info
-DCMAKE_INSTALL_MANDIR=/nix/store/86dhaq2f3kh3fy5ck8szj7xl68lhagii-liboqs-0.10.0/share/man
-DCMAKE_INSTALL_OLDINCLUDEDIR=/nix/store/944qsiqygszzvj2s98swyl9bl2k7mvlb-liboqs-0.10.0-dev/include
-DCMAKE_INSTALL_INCLUDEDIR=/nix/store/944qsiqygszzvj2s98swyl9bl2k7mvlb-liboqs-0.10.0-dev/include
-DCMAKE_INSTALL_SBINDIR=/nix/store/86dhaq2f3kh3fy5ck8szj7xl68lhagii-liboqs-0.10.0/sbin
-DCMAKE_INSTALL_BINDIR=/nix/store/86dhaq2f3kh3fy5ck8szj7xl68lhagii-liboqs-0.10.0/bin
-DCMAKE_INSTALL_NAME_DIR=/nix/store/86dhaq2f3kh3fy5ck8szj7xl68lhagii-liboqs-0.10.0/lib
-DCMAKE_POLICY_DEFAULT_CMP0025=NEW
-DCMAKE_OSX_SYSROOT=
-DCMAKE_FIND_FRAMEWORK=LAST
-DCMAKE_STRIP=/nix/store/8mjb3ziimfi3rki71q4s0916xkm4cm5p-gcc-wrapper-13.2.0/bin/strip
-DCMAKE_RANLIB=/nix/store/8mjb3ziimfi3rki71q4s0916xkm4cm5p-gcc-wrapper-13.2.0/bin/ranlib
-DCMAKE_AR=/nix/store/8mjb3ziimfi3rki71q4s0916xkm4cm5p-gcc-wrapper-13.2.0/bin/ar
-DCMAKE_C_COMPILER=gcc
-DCMAKE_CXX_COMPILER=g++
-DCMAKE_INSTALL_PREFIX=/nix/store/86dhaq2f3kh3fy5ck8szj7xl68lhagii-liboqs-0.10.0
-DBUILD_SHARED_LIBS=ON
-DOQS_DIST_BUILD=ON
-DOQS_BUILD_ONLY_LIB=ON
Here is the full logs as a gist: https://gist.github.com/Sigmanificient/28ddd430f5a439f3a703b13d12818dc9
Thanks.
-DCMAKE_INSTALL_PREFIX=/nix/store/86dhaq2f3kh3fy5ck8szj7xl68lhagii-liboqs-0.10.0
-DCMAKE_INSTALL_LIBDIR=/nix/store/86dhaq2f3kh3fy5ck8szj7xl68lhagii-liboqs-0.10.0/lib
So CMAKE_INSTALL_LIBDIR with absolute path is added automatically by Nix build system. But this is "not recommended" is the docs. Perhaps, bug report should be filed to Nix authors to fix this? Or at least their opinion should be consulted. But I understand that some packages would be broken by the either choice, so they may be reluctant to change anything.
And it looks like cmake is smart enough to strip LIBDIR prefix from CMAKE_INSTALL_FULL_LIBDIR
.
Well, in that case, it maybe more portable to patch liboqs.pc.in
to use CMAKE_INSTALL_FULL_*
s.
ps. I am curious how other Nix packages workaround this issue.
JFYI
ps. I am curious how other Nix packages workaround this issue.
Looking at https://github.com/NixOS/nixpkgs I see a lot of setting like this (overriding default values):
cmakeFlags = [
..
"-DCMAKE_INSTALL_LIBDIR=lib"
"-DCMAKE_INSTALL_INCLUDEDIR=include"
nixpkgs (master)$ git grep -e "-DCMAKE_INSTALL_LIBDIR=lib" | wc -l
114
Also sometimes they patch the sources to use CMAKE_INSTALL_FULL_LIBDIR
or use postPatch
to fix .pc
files.
postPatch = ''
substituteInPlace cmake/libappimage.pc.in \
--replace 'libdir=''${prefix}/@CMAKE_INSTALL_LIBDIR@' 'libdir=@CMAKE_INSTALL_FULL_LIBDIR@' \
--replace 'includedir=''${prefix}/@CMAKE_INSTALL_INCLUDEDIR@' 'includedir=@CMAKE_INSTALL_FULL_INCLUDEDIR@'
'';
nixpkgs (master)$ git grep -e "CMAKE_INSTALL_FULL_LIBDIR" | wc -l
124
So it seems that experienced Nix packagers already aware of the problem and workaround it on their own.
Thanks very much for this thorough analysis of Nix packaging, @vt-alt . I take from this that liboqs
doesn't have to change anything and will close the issue if there's no howls of protest.
Describe the bug Hi, while upgrading nixpkgs's liboqs (
0.8.0
->0.10.0
), i ran into an issue concerning the generatedliboqs.pc
file. Nix kindly reported the following malformation:Reverting this commit fixes the problem, generating the proper
pc
file:Environment (please complete the following information):
NixOS 23.11
3.0.13
gcc 13.2.0
"-DOQS_DIST_BUILD=ON"
"-DOQS_BUILD_ONLY_LIB=ON"
0.10.0