Respect relative/absolute CMAKE_INSTALL_INCLUDEDIR

[r-burns]

CMAKE_INSTALL_INCLUDEDIR is already used in some places, but not consistently, causing headers to be installed to multiple locations when it is specified differently from the default "include" value.

Additionally, upstream GNUInstallDirs documentation notes that install dirs may be specified as relative (to install prefix) or absolute paths, so we should only append to the install prefix when non-absolute paths are given.

[TheAssassin]

I wonder whether you'd have to use something like CMAKE_INSTALL_REL_LIBDIR then. IIRC, GNUInstallDirs uses absolute paths.

[r-burns]

They're typically relative, but they're certainly allowed to be absolute, as documented here: https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html#result-variables

It seems that in recent releases they've added that absolute paths are "discouraged" as they do not work with certain options, but absolute GNUInstallDirs are heavily relied on by distro packagers such as nixpkgs. They're used when packaging with "split outputs" (e.g. putting development components such as headers and pkg-config files in a separate prefix).

[TheAssassin]

Then I think in install_interface, we need to use the relative paths, too. Otherwise, installing with DESTDIR breaks for libappimage which some projects require. install_interface should anyway be replaced by CMAKE_PREFIX_PATH during installation.

[r-burns]

I think the INSTALL_INTERFACE value for the INCLUDE_DIRECTORIES is OK as-is. I've used it like this a lot before, and cmake docs seem to agree, saying:

Specified include directories may be absolute paths or relative paths

https://cmake.org/cmake/help/latest/command/target_include_directories.html

You just need to be aware that DESTDIR won't work with absolute GNUInstallDirs, but I don't think that breaks any existing use-cases because I'm still using the regular CMAKE_INSTALL_INCLUDEDIR here which will be relative for anyone using DESTDIR.

[TheAssassin]

Maybe doesn't break yours, but I (and some other people) run CI jobs that require full compatibility with DESTDIR.

[TheAssassin]

but I don't think that breaks any existing use-cases because I'm still using the regular CMAKE_INSTALL_INCLUDEDIR here which will be relative for anyone using DESTDIR.

How can you be so sure of this? I mean, you kind of fix it in the pkg-config file where I'd rather expect it to be less of an issue.

Edit: please note that I'm not against this PR at all, I just want to make sure we don't unintentionally break downstream use cases.

[r-burns]

Maybe doesn't break yours, but I (and some other people) run CI jobs that require full compatibility with DESTDIR.

Oh I definitely agree, I'm saying that this shouldn't break DESTDIR, so if it does then I've missed something here. Do you have any specific DESTDIR-reliant packaging tests I can run on my end? Or are the current CI failures related? I took a quick look at them but I see the same jobs failing on master so wasn't sure.

[TheAssassin]

It's quite late, but I had a glance. Not sure why they're currently broken. Can have a look this week.

[r-burns]

No worries, and no rush. Let me know if I can help test out anything on my end.

[TheAssassin]

Sure. I'll also run tests downstream before merging.