LunarG/VulkanTools

Can't rely on jsoncpp providing cmake configs

mizvekov opened this issue · 8 comments

jsoncpp has deprecated cmake build system in favor of meson. Distros / packages are switching over, and so the cmake config cannot be relied anymore. This is a problem on Homebrew for instance.

The pkgconfig is generated in both build systems, and so we can switch over, or at least add it as a fallback, but preferably the former for simplicity.

Something like this works:

find_package(PkgConfig REQUIRED)
pkg_check_modules(jsoncpp REQUIRED IMPORTED_TARGET jsoncpp)
add_library(jsoncpp_static ALIAS PkgConfig::jsoncpp) # Or preferably don't create the alias and use the imported target directly.

Fix is simple enough. Although our build process will use the CMake build since our custom package manager relies on dependencies being built with CMake.

I'll ping you on the PR.

I mean the most convenient method would be for this PR to get merged in though:
open-source-parsers/jsoncpp#1486

Since we use jsoncpp in vulkan-profiles as well.

That seems acceptable, if it can be merged.

Note that this problem is not apple specific, even if looking only from the homebrew side, as brew supports Linux as well.

Fix is simple enough.

No it's not. I don't know why I said that.

pkg-config is NOT a common WIN32 developer tool. I'm also having issue with it on the MinGW runner as well. Also the only platform where it's an assumed default Linux|BSD (kinda*). MacOS you need to install it with brew (or some other means, but it's not a default). Which complicates our existing boostrap / SDK / documentation process for little gain on our part.

So I could add 2 paths. A pkg-config / cmake-config where we try one after the other. Which also isn't great. So overall the best approach would be for json's meson to also generate jsoncppConfig.cmake. Similar to how some of our CMake repos generate package config files...

No it's not. I don't know why I said that.

pkg-config is NOT a common WIN32 developer tool. I'm also having issue with it on MinGW runner as well. Also the only platform where it's assumed to exist is Linux|BSD. MacOS you need to install it with brew (or some other means, but it's not a default). Which complicates our existing boostrap / SDK / documentation process for little gain on our part.

I see. I don't know about the Mingw failure you are seeing, this is supposed to work there, at least if you are using a full environment like msys2.
Also, for regular WIN32 / MSVC, there exists vcpkg, which seems to support pkgconfig as well.

So I could add 2 paths. A pkg-config / cmake-config where we try one after the other. Which also isn't great. So overall the best approach would be for json's meson to also generate jsoncppConfig.cmake. Similar to how some of our CMake repos generate package config files...

Sure, either approach LGTM.

I really wish there was an actual standard for this.

A de-facto standard that is pretty much cross-platform would be the cmake config, but that is probably not truly cross-buildystem supported, outside of cmake and meson at least that I know of, without manual intervention from the user. For example, you can write a cmake script, that runs in standalone mode, which fishes out the information you need from the config file. That would be for example one way folks writing a Makefile manually could use it.

I see. I don't know about the Mingw failure you are seeing, this is supposed to work there, at least if you are using a full environment like msys2.
Also, for regular WIN32 / MSVC, there exists vcpkg, which seems to support pkgconfig as well.

On the GitHub runner I get the following:

-- Found PkgConfig: C:/Strawberry/perl/bin/pkg-config.bat (found version "0.26") 
-- Checking for module 'jsoncpp'
--   Found jsoncpp, version 1.9.5

Which fails on internal CI. And isn't great either since it's kinda cheating... I fully expected this to fail.

MinGW doesn't play well with this at all

-- Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE) 
    Reason given by package: The command
      "C:/Strawberry/perl/bin/pkg-config.bat" --version
    failed with output:

    stderr: 
      Can't locate Pod/Usage.pm in @INC (you may need to install the Pod::Usage module) (@INC contains: /usr/lib/perl5/site_perl /usr/share/perl5/site_perl /usr/lib/perl5/vendor_perl /usr/share/perl5/vendor_perl /usr/lib/perl5/core_perl /usr/share/perl5/core_perl) at C:/Strawberry/perl/bin/pkg-config.bat line 1101.
    BEGIN failed--compilation aborted at C:/Strawberry/perl/bin/pkg-config.bat line 1101.
    result: 

The Windows runners don't provide pkg-config as a default unlike the linux / macos runners. And internal CI certainly doesn't.

On the GitHub runner I get the following:

-- Found PkgConfig: C:/Strawberry/perl/bin/pkg-config.bat (found version "0.26") 
-- Checking for module 'jsoncpp'
--   Found jsoncpp, version 1.9.5

Which fails on internal CI. And isn't great either since it's kinda cheating... I fully expected this to fail.

MinGW doesn't play well with this at all

-- Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE) 
    Reason given by package: The command
      "C:/Strawberry/perl/bin/pkg-config.bat" --version
    failed with output:

    stderr: 

Yeah, this is finding some weird pkg-config from a standalone windows perl distribuution, wouldn't expect this to work.
I would expect that for mingw, if you built this inside msys2, this would work fully, as that ships a fully working pkgconfig mingw port.

For pkgconfig support in windows outside of mingw/msys2, see: mesonbuild/meson#4029 (comment)
And microsoft/vcpkg#11487 (comment)

The Windows runners don't provide pkg-config as a default unlike the linux / macos runners. And internal CI certainly doesn't.

The github CI does seem to provide vcpkg by default: https://github.com/marketplace/actions/run-vcpkg

A de-facto standard that is pretty much cross-platform would be the cmake config

Looks like there is promising work actually being done to standardize this: https://www.youtube.com/watch?v=IwuBZpLUq8Q

We shall see. In the meantime I'll fix this issue Monday.