Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix CMake install dir usage in pkgconfig, honour CMAKE_INSTALL_INCLUDEDIR #258

Merged
merged 2 commits into from
Jan 3, 2023
Merged

Fix CMake install dir usage in pkgconfig, honour CMAKE_INSTALL_INCLUDEDIR #258

merged 2 commits into from
Jan 3, 2023

Conversation

OPNA2608
Copy link
Contributor

@OPNA2608 OPNA2608 commented Dec 31, 2022 •
edited
Loading

  1. Current assumption about CMAKE_INSTALL_<dir> being relative to CMAKE_INSTALL_PREFIX is wrong.
Broken paths found in a .pc file! /nix/store/0lfs3w5mgjh5rdlrzwm4h18g2jpmmygx-wlcs-1.4.0/lib/pkgconfig/wlcs.pc
The following lines have issues (specifically '//' in paths).
2:exec_prefix=${prefix}//nix/store/0lfs3w5mgjh5rdlrzwm4h18g2jpmmygx-wlcs-1.4.0/bin
3:libexecdir=${prefix}//nix/store/0lfs3w5mgjh5rdlrzwm4h18g2jpmmygx-wlcs-1.4.0/libexec
It is very likely that paths are being joined improperly.
ex: "${prefix}/@CMAKE_INSTALL_LIBDIR@" should be "@CMAKE_INSTALL_FULL_LIBDIR@"
Please see https://github.com/NixOS/nixpkgs/issues/144170 for more details.

CMAKE_INSTALL_<dir> must not be assumed to be relative to CMAKE_INSTALL_PREFIX, according to the CMake documentation:

It should typically be a path relative to the installation prefix so that it can be converted to an absolute path in a relocatable way (see CMAKE_INSTALL_FULL_<dir>). However, an absolute path is also allowed.

CMake has special _FULL_ variants of the variables which apply the prefix when needed:

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.

  1. CMAKE_INSTALL_INCLUDEDIR isn't honoured.

CMake has CMAKE_INSTALL_INCLUDEDIR for specifying where header files shall be installed to. It defaults to include (relative to CMAKE_INSTALL_PREFIX) so when not specifying an include dir, this is functionally identical to the current hardcoding.

AlanGriffiths
AlanGriffiths approved these changes Jan 3, 2023
View reviewed changes
Copy link
Contributor

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

bors merge

@bors
Copy link
Contributor

bors bot commented Jan 3, 2023

Build succeeded:

@bors bors bot merged commit 4ebc312 into canonical:main Jan 3, 2023
This was referenced Jan 13, 2023
Merged
Merged
bors bot added a commit to canonical/mir that referenced this pull request Jan 20, 2023
@bors @OPNA2608
Merge #2786
7606aa9 
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
@bors @OPNA2608
Merge #2786
58c4ca6 
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 that referenced this pull request Feb 7, 2023
@bors @OPNA2608
Merge #260
03237c9 
260: pkgconfig: Use better path concatenation r=RAOF a=OPNA2608

A correction to #258, based on recommendations from [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).

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.

I've checked that this gives regular `${prefix}/<dir>` entries again when the `CMAKE_INSTALL_*DIR`s are relative, and the `CMAKE_INSTALL_*DIR`s verbatim when they are absolute.

Co-authored-by: OPNA2608 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlanGriffiths AlanGriffiths AlanGriffiths approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@OPNA2608 @AlanGriffiths