pkgconfig: Fix path relocation for mingw

[Biswa96]

Just separate the path to prefix and includedir variables.

---

Pull request checklist

Read the Contribution Guidelines for detailed information.

• Changes are described in the pull request, or an existing issue is referenced.
• The test suite compiles and runs without error.
• Code coverage is 100%. Test cases can be added by editing the test suite.
• The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

[Biswa96]

@doronbehar Is this allowed according to the rules?

[nlohmann]

Thanks for your contribution! Is this also related to #2907?

[coveralls]

Coverage remained the same at 100.0% when pulling ec83b7b on Biswa96:pkgconfig-mingw into 4fc7b3d on nlohmann:develop.

[Biswa96]

I am not sure if this change is related to that issue. I can not reproduce that issue in my Arch installation. But I would not like to ruin it 😅 I can wait for their comments @dvzrv @eli-schwartz

This change is for msys2/mingw environment. For example, with the prefix variable, pkgconfig automatically changes/relocates /mingw64/include path to C:\msys64\mingw64\include (assuming C:\msys64 is the install directory of msys2). BTW, the variables are documented here https://people.freedesktop.org/~dbn/pkg-config-guide.html

[nlohmann]

As I have no experience with pkgconfig, but see several issues on this: is there a way I can test this in the CI? We are using mingw in GitHub Actions.

[Biswa96]

I am not sure what to test. But with this change, the output of a command will be like this:

$ pkg-config -cflags nlohmann_json
-IF:/msys64/ucrt64/include

Without this change, output is nothing.

[nlohmann]

Do I need to install anything before this?

[eli-schwartz]

This change is generally speaking correct. Although the prefix variable is not actually being used, because you set the includedir variable to cmake's "full" includedir anyway.

When configured via -DCMAKE_INSTALL_PREFIX=/usr -DCMAKE_INSTALL_INCLUDEDIR=include it's possible (and preferred) to say in the pkg-config file:

prefix=/usr
includedir=${prefix}/include

But this will be wrong in the case of -DCMAKE_INSTALL_PREFIX=/usr -DCMAKE_INSTALL_INCLUDEDIR=/usr/include:

prefix=/usr
includedir=${prefix}/usr/include

Cmake doesn't have a way of correctly handling this. Meson has join_paths('${prefix}', get_option('includedir') which correctly handles edge cases in path handling and won't add ${prefix} if the includedir is already an absolute path. (But also, Meson's pkg-config.generate() function automatically generates a .pc file for you that handles this.)

This PR is to my knowledge the best way to handle the issue in cmake, and perfect is the enemy of the good, so... It looks good to me.

[eli-schwartz]

It is NOT related to the other issue. The other issue is a problem where running the testsuite will (somehow) cause the nlohmann_json project options (specifically, the prefix and includedir) to be overwritten by the testsuite's project options.

[Biswa96]

Although the prefix variable is not actually being used

At first, I thought about this

includedir=${prefix}/@CMAKE_INSTALL_INCLUDEDIR@

But in the previous commit, it was changed to CMAKE_INSTALL_FULL_INCLUDEDIR by @doronbehar. So, I assume full includedir is the rule.

[eli-schwartz]

Right, and it links to https://github.com/jtojnar/cmake-snips#assuming-cmake_install_dir-is-relative-path which is really just another way of examining what I just mentioned.

In fact, right below that is https://github.com/jtojnar/cmake-snips#concatenating-paths-when-building-pkg-config-files which offers a cmake module reimplementing meson's built-in join_paths() function. This can solve the issue fully, "the right way".

[Biswa96]

OK, understood.

[eli-schwartz]

Hmm? Can't you just update this PR to switch to the other method? :) Why did you close it?

[Biswa96]

I am not sure if that method is perfect in every case.

[eli-schwartz]

It should be, but either that or this PR in its current state would be useful improvements over the status quo.

The perfect is the enemy of the good, hence this PR in its current state could still be merged.