build: fix pkg-config file generation #9953
Conversation
|
Hi @Tachi107! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Ah, thanks for the cleanup! First time trying to generate PC files from CMake, so was bound to get a few things not the cleanest. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Converted to draft as a few things don't work as expected Edit: forgot the |
- Instead of hardcoding "lib" and "include" in libdir and includedir, use the values from GNUInstallDirs. - Use PROJECT_DESCRIPTION and PROJECT_HOMEPAGE_URL instead of their CMAKE_ conterparts to fix pkg-config generation when rocksdb is not the top-level project. - Drop explicit CMAKE_CURRENT_SOURCE_DIR and CMAKE_CURRENT_BINARY_DIR in configure_file() as that's implied by default (and quite intuitive).
Tachi107
force-pushed
the
cmake-pkg-config-fix
branch
from
db81506 to
527e993
Compare
|
Some of us recently put in the effort to add pkg-config support to HdrHistogram_c which is also CMake-based. From the looks of it, your pkg-config file has a pitfall which was corrected in the HdrHistogram_c repo in this commit: HdrHistogram/HdrHistogram_c@b083efd. You may want to do a similar fix. |
Hi, thanks for the feedback :) @jtojnar's JoinPaths.cmake says that Windows is not supported (even though by looking at the code I don't understand why). Fixing generation of pkg-config files when absolute directories are specified at the cost of breaking Windows support doesn't look like a good trade off to me |
|
you can just special case Windows to do what you are currently doing. # invalid since I don't write cmake
if (IS_WINDOWS)
SET(include_dir_for_pc_file, ${CMAKE_INSTALL_INCLUDEDIR})
SET(libdir_dir_for_pc_file, ${CMAKE_INSTALL_LIBDIR})
else
join_paths(includedir_for_pc_file "\${prefix}" "${CMAKE_INSTALL_INCLUDEDIR}")
join_paths(libdir_for_pc_file "\${prefix}" "${CMAKE_INSTALL_LIBDIR}")
endifAre pkg-config files even used on Windows with Windows paths? |
I wrote a bit about it here: jtojnar/cmake-snips#4 TLDR: Windows has two more path types (absolute paths without drive letter and relative paths with drive letter) and I have no idea how it behaves there. Then there is using If you are able to raise minimum CMake version to 3.20, you can replace the custom module with |
|
@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Shame this got pulled in even though it is broken, imo. Unless imported means something other than merged. Don't really know how this process works. |
Still less broken than before :) It can always be refined afterwards |
|
@Tachi107 has updated the pull request. You must reimport the pull request before landing. |
|
@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Instead of hardcoding "lib" and "include" in
libdirandincludedir, use the values fromGNUInstallDirs.Use
PROJECT_DESCRIPTIONandPROJECT_HOMEPAGE_URLinstead of theirCMAKE_conterparts to fix pkg-config generation when rocksdb is not the top-level project (seeproject()).Drop explicit
CMAKE_CURRENT_SOURCE_DIRandCMAKE_CURRENT_BINARY_DIRinconfigure_file()as that's implied by default (and quite intuitive).See #9945
CC: @trynity