Skip to content

Fix pkgconfig file's libdir value#326

Merged
brarcher merged 2 commits intolibcheck:masterfrom
mattst88:fix-libdir
Mar 20, 2021
Merged

Fix pkgconfig file's libdir value#326
brarcher merged 2 commits intolibcheck:masterfrom
mattst88:fix-libdir

Conversation

@mattst88
Copy link
Copy Markdown
Contributor

@mikkoi
Copy link
Copy Markdown
Contributor

mikkoi commented Mar 13, 2021

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

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

mikkoi commented Mar 15, 2021

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

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

mikkoi commented Mar 15, 2021

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. [..]

Comment thread CMakeLists.txt Outdated
mattst88 and others added 2 commits March 19, 2021 16:55
@mattst88
Copy link
Copy Markdown
Contributor Author

==> 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
Copy link
Copy Markdown
Contributor

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 brarcher merged commit 9e714b7 into libcheck:master Mar 20, 2021
@brarcher
Copy link
Copy Markdown
Contributor

Thanks for the fix!

@mattst88 mattst88 deleted the fix-libdir branch March 20, 2021 19:15
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