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

Add a target to RapidJSONConfig.cmake.in #1350

Merged
merged 1 commit into from Mar 30, 2020
Merged

Add a target to RapidJSONConfig.cmake.in #1350

merged 1 commit into from Mar 30, 2020

Conversation

tchernobog
Copy link
Contributor

This way, users can call target_link_libraries against the imported target, which is the recommended way of doing things.

This way, users can call target_link_libraries against the imported target, which is the recommended way of doing things.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.001%) to 99.922% when pulling ce635ce on tchernobog:patch-1 into 73063f5 on Tencent:master.

@tchernobog
Copy link
Contributor Author

Any hope to see this merged?

@miloyip
Copy link
Collaborator

miloyip commented Nov 22, 2018

Can you explain why this is needed?

@tchernobog
Copy link
Contributor Author

Sure. Modern CMake is moving away from simple variables and towards providing targets. You can find the same trend in most Find*.cmake modules provided by upstream.

The reason is that variables are easy to mistype and might silently expand to nothing (target existance is instead checked at the point of usage). You also need more commands to use them, and makes for worse extensibility (you need a variable for includes, one for libraries to link against, one for compile definitions...).

Instead with targets, you just link against a target, and all sensible properties are propagated automatically.

This makes for less code to maintain and check. See also:
https://pabloariasal.github.io/2018/02/19/its-time-to-do-cmake-right/

@zhihaoy
Copy link

zhihaoy commented Jan 29, 2019

Please merge something like this before releasing a new version. It's just a new practice in CMake usage. For example, when vcpkg installs "xsimd," after finishes, it prints

    find_package(xsimd CONFIG REQUIRED)
    target_link_libraries(main PRIVATE xsimd)

Even though xsimd is header-only. But with rapidjson, it prints

    find_package(RapidJSON CONFIG REQUIRED)
    target_include_directories(main PRIVATE ${RAPIDJSON_INCLUDE_DIRS})

Which looks sort of outdated.

I kind of prefer the RapidJSON::RapidJSON target though as suggested in https://pabloariasal.github.io/2018/02/19/its-time-to-do-cmake-right/ , as it doesn't conflict with projects that already import rapidjson target by including it somewhere in their source directory.

@aphecetche
Copy link

Hi there,

This would be really really useful indeed. That way projects that use RapidJSON and modern CMake techniques would not need to defined their own FindRapidJSON.cmake "just" to define the missing RapidJSON::RapidJSON target.

Thanks,

@Kaffeine
Copy link

Kaffeine commented Mar 29, 2020

Hi dear @miloyip, thank you for your work!

The library is great as it is, but sadly it is missing two important things:

  1. RapidJSON::RapidJSON CMake target (with exported includes, like in this PR)
  2. A new release. :-)

@tchernobog, please add RapidJSON::RapidJSON target.
Once merged this will make happy a number of CMake & RapidJSON users!

@miloyip miloyip merged commit f376690 into Tencent:master Mar 30, 2020
@tchernobog tchernobog deleted the patch-1 branch March 30, 2020 16:37
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.

None yet

6 participants