libcheck / check

A unit testing framework for C
GNU Lesser General Public License v2.1
1.07k stars 209 forks source link

Fix pkgconfig file's libdir value #326

Closed mattst88 closed 3 years ago

mattst88 commented 3 years ago

Bug: https://bugs.gentoo.org/729642

mikkoi commented 3 years ago

Hi. Thanks for this fix. I read through the bug discussion but I still didn't understand what exactly had failed. What value did you except it to have, besides lib, the default.

If we make this change, should we also change: includedir=${prefix}/include to includedir=${prefix}/${CMAKE_INSTALL_INCLUDEDIR} ?

BTW, the check.pc file uses variables. AFAIK this is common, but is it also a problem? Should they rather be constant strings? Since the file is generated anyhow, would it make sence?

mattst88 commented 3 years ago

Thanks for taking a look!

What value did you except it to have, besides lib, the default.

So currently, check.pc contains:

$ grep 'Libs:' /usr/lib*/pkgconfig/check.pc
/usr/lib64/pkgconfig/check.pc:Libs: -L/usr/lib -lcheck
/usr/lib/pkgconfig/check.pc:Libs: -L/usr/lib -lcheck

which means that when you have it installed pkgconfig prints -L... for if the path is a non-default search path.

$ /usr/bin/i686-pc-linux-gnu-pkg-config --libs check
-lcheck 
$ /usr/bin/x86_64-pc-linux-gnu-pkgconf --libs check
-L/usr/lib -lcheck

This would cause a 64-bit compilation that links with -lcheck to search for -lcheck in /usr/lib (the directory containing 32-bit shared objects).

To answer your question, it should contain the default path (or no path at all).

(In Gentoo, this was further confused by seding out ${libdir} for /usr/lib64, which caused 32-bit builds to attempt to link with 64-bit shared objects, but that's neither here nor there)

If we make this change, should we also change: includedir=${prefix}/include to includedir=${prefix}/${CMAKE_INSTALL_INCLUDEDIR} ?

Hmm. I don't know. I've never seen includedir be anything other than include. I guess it wouldn't hurt.

BTW, the check.pc file uses variables. AFAIK this is common, but is it also a problem? Should they rather be constant strings?

I don't think so. Seems like using variables isn't a problem—I see lots of variables in my /usr/lib64/pkgconfig/*.pc.

mikkoi commented 3 years ago

I can see a possible corner case.

 set(libdir "\${exec_prefix}/${CMAKE_INSTALL_LIBDIR}")

could result with libdir becoming /. If that happens, would we get -L/?

mattst88 commented 3 years ago

Thanks. Other Gentoo developers pointed me to how json-c handles pkgconfig file generation: https://github.com/json-c/json-c/blob/041cef434afe0d0c6da8b6ac1d1fa26087246dda/CMakeLists.txt#L500 and independently indicated that I should use CMAKE_INSTALL_FULL_LIBDIR instead.

mikkoi commented 3 years ago

Great. The CMake documentation on GNUInstallDirs clarifies the variable:

CMAKE_INSTALL_FULL_<dir>

The absolute path generated from the corresponding CMAKE_INSTALL_<dir> value. If the value is not already an absolute path, an absolute path is constructed typically by prepending the value of the CMAKE_INSTALL_PREFIX variable. [..]
mattst88 commented 3 years ago
==> Downloading http://mirror.ctan.org/systems/mac/mactex/mactex-20200407.pkg
==> Downloading from https://mirrors.rit.edu/CTAN/systems/mac/mactex/mactex-20200407.pkg
curl: (18) transfer closed with 943584178 bytes remaining to read
Error: Download failed on Cask 'mactex' with message: Download failed: http://mirror.ctan.org/systems/mac/mactex/mactex-20200407.pkg
Error: Process completed with exit code 1.

Good grief. CI downloads a 3.9GB package?

brarcher commented 3 years ago

Yeah.... the LaTeX package is big, and is needed to test that the docs build. We don't own the image that runs the tests, so we need to configure the image each time.

Probably testing that the docs build on both Linux and OSX is unnecessary, as success on one should be sufficient. I'll look to remove the OSX version of the test a little later. I think that it downloads a larger payload than Linux.

brarcher commented 3 years ago

Thanks for the fix!