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

CMake Configuration Exporting #1309

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

grabusr
Copy link

@grabusr grabusr commented Nov 14, 2023

Feature

Allows to build CMake install target that exports GodotCppConfig.cmake allowing to find it by using find_package() function.

Usage

Configure godot-cpp CMake project in build directory:

# example project configuration
cmake .. -G Ninja -DCMAKE_INSTALL_PREFIX=<path where you want install godot_cpp>
# install
cmake --build . --target install

Now instead of storing godot_cpp project as submodule in GDExtension CMake project we can:

Configuring GDExtension project in build directory

cmake .. -G Ninja -DCMAKE_PREFIX_PATH=<path where godot_cpp was installed by install target if not in PATH env var>

Finding and linking to godot_cpp library:

find_package(GodotCpp REQUIRED)
add_library(MyExtension MODULE)
target_link_libraries(MyExtension PRIVATE godot::cpp)

Why?

  1. Storing godot-cpp as submodule can be problematic, because it is needed to compile it every time when GDExtension is compiled from scratch, This is painfull especially in CI.
  2. CMake Configuration Exporting is easy way to collect all necessary files to use in CMake based GDExtension project.
  3. Having export configuration mechanism makes creating packages for package managers like vcpkg and conan very easy.
  4. Exporting can be also used as RUN Docker command in Dockerfile. Stored built godot-cpp in Docker image allows to use it in separate environment (eg. on CI) and save time and resources.

Example install godot-cpp in Dockerfile

Tip: /usr/local/ is by default in Linux env in PATH, so when GDExtension CMake project is configured we do not need to provide CMAKE_PREFIX_PATH.

RUN git clone https://github.com/godotengine/godot-cpp.git -b godot-4.2.1-stable \
    && mkdir -p godot-cpp/build \
    && cd godot-cpp/build \
    && cmake .. -G Ninja -DCMAKE_BUILD_TYPE="Release" -DCMAKE_INSTALL_PREFIX=/usr/local/ \
    && cmake --build . --target install \
    && cd ../.. \
    && rm -rf godot-cpp

@grabusr grabusr requested a review from a team as a code owner November 14, 2023 20:20
@dsnopek dsnopek added enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup labels Nov 15, 2023
@dsnopek dsnopek added this to the 4.x milestone Nov 15, 2023
@dsnopek
Copy link
Contributor

dsnopek commented Nov 15, 2023

Thanks!

At a high-level, this makes sense: you can build the necessary version of godot-cpp into a Docker image, and then re-use it during CI. And I tested it using the commands in the description, and it seemed to work fine!

However, personally, I don't know cmake very well, so I can't say if this is the right/best way to accomplish this. Also, because I don't know cmake very well, I'm a little wary of having to maintain this long-term. So, I think it'd be good to get a 2nd opinion from someone on the GDExtension team with more cmake experience.

@notskm
Copy link

notskm commented Nov 19, 2023

I actually just did something very similar to this PR last night. I wanted godot-cpp packaged in vcpkg for my project, but I needed these exports to do it.

When we link against a library in CMake, the official and recommended way is to use find_package(CONFIG). It works in all situations, so it's absolutely critical to have proper find_package support. It's not just this Docker situation that benefits. find_package(CONFIG) is suggested when you're using a library as a subdirectory of your project, via a package manager, or in any other situation. It is the way you consume libraries in CMake.

At a glance, I do see some odd things going on here. There should be no reason to add GODOT_CPP_INSTALL as an option. The install commands set up the ability to run cmake --install .... There should be no need to opt in or out.

It is also suggested to use the GNUInstallDirs to set up the install locations.

I can take a deeper look at this when I have some more time if you'd like. It's easy to miss details, so it warrants a few more eyes, but if we get it right the first time, it shouldn't need to be modified often.

@grabusr
Copy link
Author

grabusr commented Nov 30, 2023

If you use godot-cpp project as subproject (eg. in your GDExtension CMake project) and you use install target in case:

add_subdirectory(godot-cpp)

#...

add_library(MyExtension)
target_link_libraries(MyExtension PRIVATE godot::cpp)

install(TARGETS MyExtension ...)

then installing GDExtension project will export not only MyExtension.so (or *.dll), but also godot-cpp's library and config which in most of cases is not expected behavior.

To avoid that there is a common practice in CMake for libraries/frameworks exporting config to introduce flag like _INSTALL, eg. like in Google Test we have a cache flag INSTALL_GTEST.

If we use godot-cpp CMake project as sub-project we set cache flag before calling add_subdirectory(godot_cpp):

set(GODOT_CPP_INSTALL OFF CACHE BOOL "" FORCE)
add_subdirectory(godot_cpp)

Now building target install export only our GDExtension binaries (or other resources if needed) without exporting godot_cpp library and CMake config files.

Regarding install locations, targets by default use GNUInstallDirs, only header files need to be setup here. I try to use it by assigning include directories as PUBLIC_HEADERS next week.

@grabusr
Copy link
Author

grabusr commented Dec 23, 2023

I have been playing with PUBLIC_HEADER property. It is just adding few commands:

if (GODOT_GDEXTENSION_DIR AND EXISTS ${GODOT_GDEXTENSION_DIR})
	file(GLOB_RECURSE GODOT_GDEXTENSION_HEADERS CONFIGURE_DEPENDS ${GODOT_GDEXTENSION_DIR}/*.h**)
endif ()

set(PUBLIC_HEADERS_LIST
		${GENERATED_FILES_LIST}
		${HEADERS}
		${GODOT_GDEXTENSION_HEADERS}
)
set_target_properties(${PROJECT_NAME} PROPERTIES PUBLIC_HEADER "${PUBLIC_HEADERS_LIST}")

and it install all header files in CMAKE_INSTALL_INCLUDEDIR. The problem is that it does not keep directory structure, every header file is copied directly under include directory. Also using variable CMAKE_INSTALL_INCLUDEDIR does not solve the problem, because this:

install(DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/include/
	DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
)

install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/gen/include/
	DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
)

results in CMake project loading error, because during project configuration variable ${CMAKE_INSTALL_INCLUDEDIR} is empty. So if we would like to keep directory structure we need to use install(DIRECTORY ... DESTINATION include) command.

@dsnopek dsnopek added the cmake label Jan 3, 2024
@ytnuf
Copy link
Contributor

ytnuf commented Mar 4, 2024

This is great! I would like something like this, so I can use this library with package managers like vcpkg.

That said I feel like the export name should be be consistent with the alias name, which is godot::cpp.

A library user would expect the library's target name to be the same regardless of how it's consumed.
So I think it's best to upheld that assumption.

Fortunately the fix is only 1 line, just add EXPORT_NAME "cpp" here
https://github.com/grabusr/godot-cpp/blob/6b02a58e5a21fa8103abac8953d029e130d750f0/CMakeLists.txt#L212

set_target_properties(${PROJECT_NAME}
	PROPERTIES
		CXX_EXTENSIONS OFF
		POSITION_INDEPENDENT_CODE ON
		ARCHIVE_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/bin"
		LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/bin"
		RUNTIME_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/bin"
		OUTPUT_NAME "${OUTPUT_NAME}"
		EXPORT_NAME "cpp"
)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants