pkgconfig: Use better path concatenation #260
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Unconditionally using `CMAKE_INSTALL_FULL_*DIR`s in a pkg-config file is slightly wrong, because it is expected that the variables are made up of other variables, all of which can be overridden by the user. CMake added a `cmake_path` function which can do the required concatenation without the added function, but it's only available on version 3.20 while the current CMakeLists specifies a minimum version of 3.5.
bors bot
added a commit
to canonical/mir
that referenced
this pull request
Jan 20, 2023
2786: Better concatenation for pkg-config values, better use of GNUInstallDirs variables r=RAOF a=OPNA2608 Similar situation as canonical/wlcs#258, canonical/wlcs#260: - CMake code assumes `CMAKE_INSTALL_<dir>` are relative to `CMAKE_INSTALL_PREFIX` when they may be absolute and completely outside of the `PREFIX`, blindly concatenating these two causes broken pkg-config files in such situations - `CMAKE_INSTALL_FULL_<dir>` should only be used in pkg-config files when the dir is not relative to the `${prefix}` so overriding the pkg-config variables has the intended effect - In various places, suitable GNUInstallDirs variables for specifying header & data file locations are ignored - As a minor point `FULL` & non-`FULL` variables are used inconsistently in `install` commands I've - fixed these concatenations in CMake & pkg-config files according to [jtojnar/cmake-snips#concatenating-paths-when-building-pkg-config-files](https://github.com/jtojnar/cmake-snips/tree/a9d72829ab0cb42e541c9975e698d6b8282610ed#concatenating-paths-when-building-pkg-config-files) - added more usage of `CMAKE_INSTALL_INCLUDEDIR` (defaults to `include`) & `CMAKE_INSTALL_DATADIR` (`share`) and - replaced some `FULL` variables with non-`FULL` variants where the effect should be the same, for consistency I've cherry-picked these commits onto 2.11.0 and checked the pkg-config files with relative & absolute install paths, resulting files looked okay to me. I've also tried building egmde against it and it builds & launches, where previously it would fail to find Mir's headers if `CMAKE_INSTALL_INCLUDEDIR` was an absolute path. Co-authored-by: OPNA2608 <[email protected]>
bors bot
added a commit
to canonical/mir
that referenced
this pull request
Jan 20, 2023
2786: Better concatenation for pkg-config values, better use of GNUInstallDirs variables r=RAOF a=OPNA2608 Similar situation as canonical/wlcs#258, canonical/wlcs#260: - CMake code assumes `CMAKE_INSTALL_<dir>` are relative to `CMAKE_INSTALL_PREFIX` when they may be absolute and completely outside of the `PREFIX`, blindly concatenating these two causes broken pkg-config files in such situations - `CMAKE_INSTALL_FULL_<dir>` should only be used in pkg-config files when the dir is not relative to the `${prefix}` so overriding the pkg-config variables has the intended effect - In various places, suitable GNUInstallDirs variables for specifying header & data file locations are ignored - As a minor point `FULL` & non-`FULL` variables are used inconsistently in `install` commands I've - fixed these concatenations in CMake & pkg-config files according to [jtojnar/cmake-snips#concatenating-paths-when-building-pkg-config-files](https://github.com/jtojnar/cmake-snips/tree/a9d72829ab0cb42e541c9975e698d6b8282610ed#concatenating-paths-when-building-pkg-config-files) - added more usage of `CMAKE_INSTALL_INCLUDEDIR` (defaults to `include`) & `CMAKE_INSTALL_DATADIR` (`share`) and - replaced some `FULL` variables with non-`FULL` variants where the effect should be the same, for consistency I've cherry-picked these commits onto 2.11.0 and checked the pkg-config files with relative & absolute install paths, resulting files looked okay to me. I've also tried building egmde against it and it builds & launches, where previously it would fail to find Mir's headers if `CMAKE_INSTALL_INCLUDEDIR` was an absolute path. Co-authored-by: OPNA2608 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A correction to #258, based on recommendations from jtojnar/cmake-snips#concatenating-paths-when-building-pkg-config-files.
Unconditionally using
CMAKE_INSTALL_FULL_*DIRs in a pkg-config file is slightly wrong, because it is expected that the variables are made up of other variables, all of which can be overridden by the user.CMake added a
cmake_pathfunction which can do the required concatenation without the added function, but it's only available on version 3.20 while the current CMakeLists specifies a minimum version of 3.5.I've checked that this gives regular
${prefix}/<dir>entries again when theCMAKE_INSTALL_*DIRs are relative, and theCMAKE_INSTALL_*DIRs verbatim when they are absolute.