Refactor how CMake uses external flatc and generates flatbuffers targets #8008
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
| WORKING_DIRECTORY "${working_dir}" | ||
| COMMENT "Building ${schema} flatbuffers...") | ||
| list(APPEND all_generated_header_files ${generated_include}) | ||
| list(APPEND all_generated_source_files ${generated_source_file}) | ||
| list(APPEND generated_custom_commands "${generated_include}" "${generated_source_file}") | ||
| list(APPEND all_generated_files "${generated_include}" "${generated_source_file}") |
There was a problem hiding this comment.
Renamed generated_custom_commands -> all_generated_files because the old name was very misleading
| @@ -325,8 +337,15 @@ function(flatbuffers_generate_headers) | |||
| PREFIX "Flatbuffers/Schemas" | |||
| FILES ${FLATBUFFERS_GENERATE_HEADERS_SCHEMAS}) | |||
| if (NOT ${FLATBUFFERS_GENERATE_HEADERS_BINARY_SCHEMAS_DIR} STREQUAL "") | |||
|
|
|||
| # Resolve any relative paths for the source group call | |||
| get_filename_component(BINARY_SCHEMAS_DIR_ABSOLUTE_PATH | |||
There was a problem hiding this comment.
Bugfix: If a relative path was passed to BINARY_SCHEMAS_DIR, the call to source_group() would cause a fatal error. This makes the path absolute.
|
|
||
| if(FlatBuffers_FOUND) | ||
| # Provide imported target for the executable | ||
| add_executable(flatbuffers::flatc IMPORTED GLOBAL) |
There was a problem hiding this comment.
FindFlatBuffers now provides an imported target flatbuffers::flatc, in line with CMake best practices
| set_target_properties(flatsample PROPERTIES LINKER_LANGUAGE CXX) | ||
|
|
||
| compile_schema_for_samples(samples/monster.fbs "${FLATC_OPT_COMP}") | ||
| flatbuffers_generate_headers(TARGET flatsample |
There was a problem hiding this comment.
Minor change: CMakeLists.txt now calls the flatbuffers_generate_headers() function at least once. That way, if there are any issues with this function, they will be caught by CI
|
|
||
| # Attempt to read the current version of flatbuffers by looking at the latest tag. | ||
| include(CMake/Version.cmake) | ||
| list(APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_SOURCE_DIR}/CMake) |
There was a problem hiding this comment.
Minor change: the CMake subdirectory is now added to the CMake module path. This was needed to make find_package() work.
| # | ||
| # When the target_link_libraries is done within a different directory than |
There was a problem hiding this comment.
This is no longer needed, because the dependency is added automatically.
|
Can you fix the conflicts and i can review? |
relativityspace-jsmith
force-pushed
the
dev/refactor-cmake-external-flatc
branch
from
db5181b to
2d06262
Compare
|
@dbaileychess Done! Thank you! |
relativityspace-jsmith
force-pushed
the
dev/refactor-cmake-external-flatc
branch
from
2d06262 to
5dcacfb
Compare
relativityspace-jsmith
force-pushed
the
dev/refactor-cmake-external-flatc
branch
from
6b64a49 to
40e8e87
Compare
relativityspace-jsmith
force-pushed
the
dev/refactor-cmake-external-flatc
branch
2 times, most recently
from
174cac3 to
2189d3c
Compare
|
@dbaileychess Rebased this on master, will you have a chance to review soon? |
|
This pull request is stale because it has been open 6 months with no activity. Please comment or label |
- Add ability to build tests & samples using an external flatc executable' - Upgrade FindFlatBuffers: Now provides an imported target and version number, also remove legacy generation function - Enforce version compatibility between flatc and this source repo - Make sure all generates files get regenerated when the flatc version changes - Fix source files being added to generated flatbuffers interface targets - Fix CMake dying when specifying a relative path for include prefix
relativityspace-jsmith
force-pushed
the
dev/refactor-cmake-external-flatc
branch
from
2189d3c to
7315b97
Compare
|
Not stale, just rebased on master. |
|
not-stale |
At my company we've started to use FlatBuffers as a sub-project in an application, using CMake as a build system. Our experience has been pretty good overall! Except, adding flatbuffers as a CMake subdirectory when crosscompiling has been a bit problematic -- we actually had to make an internal fork with patches on top. I created this PR to provide some refactoring to make this sort of usage smoother. There are three main issues here, but they're all inter-related a bit, so it felt easier to fix em all at once.
"Cannot find source file" due to declaring generated files as interface sources
One big issue is that the flatbuffers_generate_headers() function adds the generated files as interface source files of the generated target, meaning that targets which try to link the FlatBuffers-generated target add them as source files. This works, right up until you try to link to the flatbuffers generated code from another directory. In CMake, generated source files are visible only in the directory where they are declared, not in other directories. So, the configuration will die with an ugly error like:
and no additional explanation of what happened.
To fix this problem, I removed the
target_sources(INTERFACE ...)call for the generated target, as there is no correct way to add interface sources to a target which come from custom commands. For the headers, those will just be found via the include directories. For the sources, if they exist, those will get compiled into the target itself now (good for preventing multiple definition issues).Verifying version compatibility between flatc and the flatbuffers library
If you are cross-compiling, you need to have
flatcin your path for the build to work, but there's no check that flatc is actually the correct version. If it's the wrong version, you won't find out until the compilation fails with a static assertion. So, I added a version check to FindFlatBuffers.cmake, and added code to CMakeLists.txt to verify that flatc and this repository have compatible versions or the build will fail with an error.While I was at it, I also added the ability to compile the tests even if FLATBUFFERS_BUILD_FLATC is disabled. Now, if FLATBUFFERS_BUILD_FLATC is FALSE, it will attempt to find flatc on the PATH.
Regenerating files when flatc changes
If you upgrade flatc, it's important for CMake to force all of the generated files to get regenerated. The current code tries to do this, unfortunately the FLATC_TARGET variable turns out to be empty when cross-compiling, so the dependency doesn't actually get added. This means that a manual clean is required for all projects whenever you change the flatc version on your machine, which is not great.
This PR fixes that by setting up the dependency correctly regardless of whether internal or external flatc is being used.
I also made a couple other small fixes, which I'll comment in the code. But, I hope this makes sense and that you'll take a look at this PR!