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

Skip find_package for already present targets #236

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

GregTheMadMonk
Copy link
Contributor

For #231.

Didn't properly test yet, and I don't know what to do with msgpack-c and PkgConfig, if anything needs to be done for them

@liuzicheng1987
Copy link
Contributor

@GregTheMadMonk , thanks for the initiative. Let me know when this is finished (it appears that this is still a draft).

@xsun2001
Copy link

xsun2001 commented Nov 8, 2024

I would be appreciate if the same support is provided to toml format for better CMake/CPM intergration.

if (REFLECTCPP_TOML)
    list(APPEND REFLECT_CPP_SOURCES
        src/reflectcpp_toml.cpp
    )
    if (TARGET tomlplusplus::tomlplusplus)
		target_link_libraries(reflectcpp PRIVATE tomlplusplus::tomlplusplus)
    else()
		if (MSVC)
	        target_link_libraries(reflectcpp PRIVATE "${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/lib/tomlplusplus${CMAKE_STATIC_LIBRARY_SUFFIX}")
	    else ()
	        target_link_libraries(reflectcpp PRIVATE "${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/lib/libtomlplusplus${CMAKE_STATIC_LIBRARY_SUFFIX}")
	    endif ()
	endif()
endif()

@liuzicheng1987
Copy link
Contributor

@GregTheMadMonk , what is status on this PR? It is still marked as a draft...is that on purpose? Do you want me to review and then potentially merge it?

@GregTheMadMonk
Copy link
Contributor Author

GregTheMadMonk commented Nov 21, 2024

@liuzicheng1987 yes, sorry, I was sidetracked by other tasks and kind of pushed this to the side.

Here I've rebased the branch to match your current main, added it for (I hope) all possible dependencies (@xsun2001 including tomlplusplus) and tried to test the libraries and how they work with this patch. Aside from the following, everything worked fine as far as I can tell:

  1. CMakeLists.txt in yyjson repo only provides a yyjson target, yyjson::yyjson needs to be provided manually as an alias after importing yyjson project. Similar situation with bson_static, bson_shared and flatbuffers. I've decided to leave it as is, as declaring an ALIAS library is not hard and this helps keep things uniform with what is expected from found packages
  2. msgpack (I used cpp_master branch) throws a no such file or directory error on #include <msgpack.h> for some reason
  3. tinycbor does not provide a CMakeLists.txt at all. I made it so that if a tinycbor target is present in the project (probably declared manually by a person who uses tinycbor with cmake), it is used instead of searching vcpkg paths. Didn't test it yet
  4. Didn't test benchmark and simdjson

Additionally, I've made it so that install and other install code is only called if reflectcpp is a toplevel project because it threw an error trying to declare exports for dependencies too. Which is probably a correct thing to do anyway, since a local dependency of some project should not be installed system-wide

IDK if I should mark it as a regular PR or leave it a draft for now. It'd be great if someone else tested and verified that it doesn't break builds for them too.

edit: Removed the "Draft" mark, since I don't think there's anything else to add here except maybe get msgpack working and at least have someone else verify it works for them too.

@GregTheMadMonk GregTheMadMonk marked this pull request as ready for review November 25, 2024 10:44
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.

3 participants