cmake: Fix paths with absolute GNUInstallDirs

[jtojnar]

It is not generally true that CMAKE_INSTALL_<dir> variables are relative paths:

https://github.com/jtojnar/cmake-snips#concatenating-paths-when-building-pkg-config-files
https://github.com/jtojnar/cmake-snips#assuming-cmake_install_dir-is-relative-path

Let's join them properly as paths, not strings.

Signed-off-by: Jan Tojnar jtojnar@gmail.com

This is a backport of #1263

[clanmills]

Thanks. I know nothing about GNUInstallDirs and will investigate this weekend.

If your PR correctly builds/installs/tests on our 8 supported platforms (macOS, Ubuntu, msvc, Cygwin64, MinGW/msys2, solaris, freebsd and netbsd), I will merge this into the code base.

[jtojnar]

Verified that the build succeeds on x86_64 NixOS with the following CMake flags:

-DCMAKE_FIND_USE_SYSTEM_PACKAGE_REGISTRY=OFF -DCMAKE_FIND_USE_PACKAGE_REGISTRY=OFF -DCMAKE_EXPORT_NO_PACKAGE_REGISTRY=ON -DCMAKE_BUILD_TYPE=Release -DCMAKE_SKIP_BUILD_RPATH=ON -DCMAKE_INSTALL_LOCALEDIR=/nix/store/m2djl01f0mr81n216svrlyyrs5q07ajh-exiv2-0.27.3/share/locale -DCMAKE_INSTALL_LIBEXECDIR=/nix/store/m2djl01f0mr81n216svrlyyrs5q07ajh-exiv2-0.27.3/libexec -DCMAKE_INSTALL_LIBDIR=/nix/store/m2djl01f0mr81n216svrlyyrs5q07ajh-exiv2-0.27.3/lib -DCMAKE_INSTALL_DOCDIR=/nix/store/9qpp92zjk7xrm12vc0jsmawqzn8gnyph-exiv2-0.27.3-doc/share/doc/exiv2 -DCMAKE_INSTALL_INFODIR=/nix/store/m2djl01f0mr81n216svrlyyrs5q07ajh-exiv2-0.27.3/share/info -DCMAKE_INSTALL_MANDIR=/nix/store/p8rzibh8z21qsbnmkbwdysyig62j2xsx-exiv2-0.27.3-man/share/man -DCMAKE_INSTALL_OLDINCLUDEDIR=/nix/store/0dlz3x38jvfm8sdqa1k1j4vrkgfl6d28-exiv2-0.27.3-dev/include -DCMAKE_INSTALL_INCLUDEDIR=/nix/store/0dlz3x38jvfm8sdqa1k1j4vrkgfl6d28-exiv2-0.27.3-dev/include -DCMAKE_INSTALL_SBINDIR=/nix/store/m2djl01f0mr81n216svrlyyrs5q07ajh-exiv2-0.27.3/sbin -DCMAKE_INSTALL_BINDIR=/nix/store/m2djl01f0mr81n216svrlyyrs5q07ajh-exiv2-0.27.3/bin -DCMAKE_INSTALL_NAME_DIR=/nix/store/m2djl01f0mr81n216svrlyyrs5q07ajh-exiv2-0.27.3/lib -DCMAKE_POLICY_DEFAULT_CMP0025=NEW -DCMAKE_OSX_SYSROOT= -DCMAKE_OSX_ARCHITECTURES=x86_64 -DCMAKE_FIND_FRAMEWORK=last -DCMAKE_STRIP=/nix/store/zp4vhfn31ky68xy0s6mssxh4c90z9v9a-binutils-2.31.1/bin/strip -DCMAKE_RANLIB=/nix/store/zp4vhfn31ky68xy0s6mssxh4c90z9v9a-binutils-2.31.1/bin/ranlib -DCMAKE_AR=/nix/store/zp4vhfn31ky68xy0s6mssxh4c90z9v9a-binutils-2.31.1/bin/ar -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ -DCMAKE_INSTALL_PREFIX=/nix/store/m2djl01f0mr81n216svrlyyrs5q07ajh-exiv2-0.27.3 -DEXIV2_ENABLE_NLS=ON -DEXIV2_BUILD_DOC=ON

Also checked that the _IMPORT_PREFIX path in lib/cmake/exiv2/exiv2Config.cmake is absolute as expected with the absolute includedir flag, and that libdir and includedir in lib/pkgconfig/exiv2.pc point to the expected location with the absolute flags.

[clanmills]

I'm testing this on the supported platforms.

Two comments. Both for discussion not fixing.

1. I wonder if this code should be put into a function with a name such as std::string joinPaths(const std::string&,const std::string&). We have to be careful on 0.27-maintenance not to change the library API, so it should be added as a static function in exiv2.cpp and types.cpp. However on 'master', we can change the API and it should probably go into src/futils.cpp

        const std::string localeDir = EXV_LOCALEDIR[0] == '/' ? EXV_LOCALEDIR : (Exiv2::getProcessPath() + EXV_LOCALEDIR);

2. rename JoinPaths.cmake as joinPaths.cmake

In general we try to avoid file name starting with Capital letters. Most of the files with Capital letters are aliens that we copied from other projects. Perhaps that applies in this case.

[clanmills]

I'm very surprised that this appears to have broken localisation. I've added debugging code to src/exiv2.cpp main():

int main(int argc, char* const argv[])
{
    Exiv2::XmpParser::initialize();
    ::atexit(Exiv2::XmpParser::terminate);

#ifdef EXV_ENABLE_NLS
    setlocale(LC_ALL, "");
    const std::string localeDir = Exiv2::getProcessPath() + EXV_LOCALEDIR;
    std::cout << "EXV_LOCALEDIR = " << EXV_LOCALEDIR << std::endl;
    std::cout << "localeDir     = " << localeDir     << std::endl;
    bindtextdomain(EXV_PACKAGE_NAME, localeDir.c_str());
    textdomain(EXV_PACKAGE_NAME);
...

And when I run build/run (without the patch)

680 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance/build $ env LANG=fr_FR exiv2
EXV_LOCALEDIR = /../share/locale
localeDir     = /usr/local/bin/../share/locale
exiv2: Une action doit être spécifié
exiv2: Au moins un fichier est nécessaire
Utilisation : exiv2  [ options ] [ action ] fichier ...

Manipulation des métadonnées EXIF issues des images.
681 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance/build $ 

Applying the patch (and change to exiv2.cpp)

692 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance/build $ env LANG=fr_FR exiv2
EXV_LOCALEDIR = ../share/locale
localeDir     = /usr/local/bin../share/locale
exiv2: An action must be specified
exiv2: At least one file is required
Usage: exiv2 [ options ] [ action ] file ...

Manipulate the Exif metadata of images.
693 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance/build $ 

That's not right!

We should also be careful about testing '/'. The header include/exiv2/config.h provides:

///// Path separator macros      /////
# if defined(WIN32) && !defined(__CYGWIN__)
#  define EXV_SEPARATOR_STR "\\"
#  define EXV_SEPARATOR_CHR '\\'
# else
#  define EXV_SEPARATOR_STR "/"
#  define EXV_SEPARATOR_CHR '/'
# endif

[jtojnar]

Hmm, weird, somehow I have lost "/" + here when rebasing (it present in both places in the original PR.

[jtojnar]

Should be fixed now.

[clanmills]

It is fixed, now. Thank You very much.

[jtojnar]

On Windows, the absolute paths will likely remain unsupported as neither the CMake function, nor the C++ code is able to detect absolute paths. Adding support for that would be too much effort, especially since people running Windows will likely want relocatable builds so they will use relative paths. For those the functionality should remain unchanged.

[clanmills]

msvc/localisation is documented as "not available". However the default vcpkg does built it and it's on my TODO list to investigate: #1255

Don't get distracted by Windows. If this builds and passes the test suite on the 8 supported platforms, I'm happy.

I'm also happy to accept JoinPaths.cmake with Capital letters as it's an alien. And let's not complicate this with a joinPaths() function in C++.

[jtojnar]

1. I wonder if this code should be put into a function with a name such as `std::string joinPaths(const std::string&,const std::string&)`.  We have to be careful on 0.27-maintenance not to change the library API, so it should be added as a static function in exiv2.cpp and types.cpp.  However on 'master', we can change the API and it should probably go into src/futils.cpp

        const std::string localeDir = EXV_LOCALEDIR[0] == '/' ? EXV_LOCALEDIR : (Exiv2::getProcessPath() + EXV_LOCALEDIR);

It would be nice if we could create a header file that would be included by both. I am bit rusty with C++ so I am not sure if it is possible to have a function in a header without becoming a part of the public API.

If we had to create a two copies of the function, each used only once, I would prefer just leaving it as expressions since they are quite simple. But I will defer to you.

1. rename JoinPaths.cmake as joinPaths.cmake

In general we try to avoid file name starting with Capital letters. Most of the files with Capital letters are aliens that we copied from other projects. Perhaps that applies in this case.

I have chosen PascalCase for the module name since that is the official CMake convention, as far as I could tell. If this project has a different convention, I can, of course, change it accordingly.

[clanmills]

Capitalisation is fashion. Let's not discuss it.

And let's not bother using a function to compute the path. This works and we're both happy to get on with other stuff on our plate. I think we'll add a joinPath() API in 'master' at some-time in the future. However not at the moment. It's working and that's sufficient for now.

Good Job fixing the localisation. Thanks very much.

[clanmills]

It has been fun working with you. Come back soon. You are always welcome.

Can you be check that the localisation changes are in 'master'.

[jtojnar]

The original pull request was fine, I made the mistake when rebasing onto 0.27 branch. I now also changed the "/" to EXV_SEPARATOR_STR to synchronize with this branch.

It has been a pleasure working with you too.