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.

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

Create a jsoncppConfig.cmake file, even if building under meson #1486

Merged
merged 5 commits into from
Sep 10, 2024

Conversation

wdouglass
Copy link
Contributor

If jsoncpp gets built under meson, no cmake package files get created. This means, if another project wants to use jsoncpp, then jsoncpp must have also been built with cmake.

I've created a shim that allows a jsoncppConfig.cmake file to be generated under a meson build. I have found this useful when building under buildroot, which uses meson.

this file only exposes the jsoncpp_lib target, not the jsoncpp_static one. I don't see a way to build the jsoncpp_static target under meson, but I am much less experienced with meson, so I am very open to suggestion here. the attached patch works for me.

@eli-schwartz
Copy link

This means, if another project wants to use jsoncpp, then jsoncpp must have also been built with cmake.

Not really true, the other project could use find_package(PkgConfig) followed by pkg_check_modules(...).

this file only exposes the jsoncpp_lib target, not the jsoncpp_static one. I don't see a way to build the jsoncpp_static target under meson, but I am much less experienced with meson, so I am very open to suggestion here. the attached patch works for me.

The static library is built by meson if you use the -Ddefault_library=static. The default is "shared". Another permissible value is "both".

Since this hardcodes .so, it will probably not work on macOS or Windows... it also hardcodes the value of get_option('libdir'), get_option('includedir')...

@wdouglass
Copy link
Contributor Author

That's fair -- i'll do another lap and resubmit

@wdouglass wdouglass marked this pull request as draft May 10, 2023 14:24
@wdouglass wdouglass marked this pull request as ready for review May 11, 2023 19:50
@wdouglass
Copy link
Contributor Author

I have reworked this pr to address your concerns

meson.build Outdated
add_library(jsoncpp_lib IMPORTED SHARED)
set_target_properties(jsoncpp_lib PROPERTIES
IMPORTED_LOCATION "${PACKAGE_PREFIX_DIR}/''' + get_option('libdir') + '/' + shared_name + '''"
INTERFACE_INCLUDE_DIRECTORIES "${PACKAGE_PREFIX_DIR}/''' + get_option('includedir') + '")')

Choose a reason for hiding this comment

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

I'd suggest using meson's join_paths('${PACKAGE_PREFIX_DIR}', get_option('XXXdir'), shared_name) here, since it handles the admittedly unlikely case that libdir or includedir aren't inside the prefix, and also means you can avoid repeatedly entering and then leaving a string literal context.

meson.build Outdated

import('cmake').configure_package_config_file(
name: 'jsoncpp',
input: 'jsoncppConfig.cmake.meson.in',
configuration: cmakeconf)
install_data('jsoncpp-namespaced-targets.cmake', install_dir : get_option('libdir') + '/cmake/' + jsoncpp_lib.name())

Choose a reason for hiding this comment

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

I'd use join_paths here too.

@@ -15,7 +15,7 @@ project(
'cpp_std=c++11',
'warning_level=1'],
license : 'Public Domain',
meson_version : '>= 0.49.0')
meson_version : '>= 0.54.0')

Choose a reason for hiding this comment

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

The fs module is available since 0.53.0, am I overlooking something else that needs 0.54.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure -- when i ran the build meson complained that I should be requiring 0.54.0

I'll do another commit (for the join_paths suggestion, thanks!) and try 0.53.0 then

Choose a reason for hiding this comment

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

In that case it's probably correct -- I haven't checked out the PR and tried it myself -- it should list which feature requires 0.54 though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like the 'name' member function was introduced in 0.54.0

WARNING: Project specifies a minimum meson_version '>= 0.53.0' but uses features which were added in newer versions:
 * 0.54.0: {'name'}

@mizvekov
Copy link

Ping, this affects some upstream users, can we merge this?

@juan-lunarg
Copy link

Ping, this affects some upstream users, can we merge this?

Currently we have to maintain 2 different code paths since we can't use pkg-config reliably for our Windows builds. We only maintain the pkg-config path to be compliant with package managers that use the meson build instead of the CMake build.

find_package(jsoncpp CONFIG)

if (TARGET jsoncpp_static)
    # RPATH causes issues for the Linux SDK tarball due to absolute paths.
    # This can be worked around with relative RPATHS but the tarball doesn't ship jsoncpp.
    # So the easiest solution is to just the static version of jsoncpp.
    target_link_libraries(vkvia PRIVATE jsoncpp_static)
    set_target_properties(vkvia PROPERTIES SKIP_BUILD_RPATH TRUE)
elseif(UNIX)
    # https://github.com/LunarG/VulkanTools/issues/1908
    find_package(PkgConfig REQUIRED)
    pkg_check_modules(jsoncpp REQUIRED IMPORTED_TARGET jsoncpp)
    target_link_libraries(vkvia PRIVATE PkgConfig::jsoncpp)
else()
    message(FATAL_ERROR "Unable to find jsoncpp!")
endif()

@eli-schwartz
Copy link

@juan-lunarg this is incorrect. pkg-config is not specific to unix. It may or may not be installed on Windows, but that is very different from "totally not available at all". It's e.g. available in vcpkg/conan, and can be manually installed anywhere but would require setting up the prefix paths manually rather than having them be set by default.

The "set manually rather than by default" part is, I'm pretty sure, the only reason why Windows users historically don't like it. Well, that and the fact that CMake spreads a lot of untrue FUD about its file format and then refuses to do the simple and obvious thing and e.g. link to libpkgconf to ensure consistency.


Please check both, without restricting the situations where you check them. Instead of setting REQUIRED, just do precisely what you do for the cmake config version, and check if the lookup was found and emit message(FATAL_ERROR) if not.

@juan-lunarg
Copy link

juan-lunarg commented Nov 13, 2023

Update never mind we can use the pkg-config path see:
LunarG/VulkanTools#1938

We still support both paths but now we actively test both paths which was my main issue before.

# https://github.com/LunarG/VulkanTools/issues/1908
find_package(PkgConfig)
if (PKG_CONFIG_FOUND)
    pkg_check_modules(jsoncpp REQUIRED IMPORTED_TARGET jsoncpp)
    target_link_libraries(vkvia PRIVATE PkgConfig::jsoncpp)
else()
    find_package(jsoncpp CONFIG REQUIRED)
    target_link_libraries(vkvia PRIVATE jsoncpp_static)
endif()

The problem was that the jsoncpp.pc was pointing to the SHARED version of the library instead of the STATIC version which is what we wanted. Disabling the SHARED version fixes the issues for us.

@eli-schwartz
Copy link

Thanks, that looks pretty reasonable. :)

@baylesj baylesj merged commit 4b1bd44 into open-source-parsers:master Sep 10, 2024
10 checks passed
SpiderX added a commit to SpiderX/portage-overlay that referenced this pull request Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants