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. |
| 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
| 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.
| # to generate the schemas, such that linking to the target passed as the TARGET argument | ||
| # will make the schema headers available. | ||
| # | ||
| # 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 |
relativityspace-jsmith
force-pushed
the
dev/refactor-cmake-external-flatc
branch
from
2189d3c to
7315b97
Compare
|
Not stale, just rebased on master. |
|
not-stale |
|
This pull request is stale because it has been open 6 months with no activity. Please comment or label |
|
not-stale |
relativityspace-jsmith
force-pushed
the
dev/refactor-cmake-external-flatc
branch
from
7315b97 to
7932be3
Compare
relativityspace-jsmith
force-pushed
the
dev/refactor-cmake-external-flatc
branch
from
e344e23 to
b4ed376
Compare
| # This is the legacy minimum version flatbuffers supported for a while. | ||
| cmake_minimum_required(VERSION 3.8...3.25.2) | ||
| cmake_minimum_required(VERSION 3.10...3.25.2) | ||
| cmake_policy(VERSION 3.10) |
There was a problem hiding this comment.
The latest versions of CMake are dropping support for CMake <3.10 compatibility and will print a warning when encountering this code. So we need to bump this a bit to CMake 3.10 (released Nov 2017).
|
hey @relativityspace-jsmith - just a heads up that before this PR can be merged, even after a review, you'll need to sign the google CLA. I can take a look into this as well soon(TM) to provide any feedback. I see you mentioned there is a windows/linux bfbs discrepancy -- is this worth spawning its own issue over? I don't really love the idea of committing a hack into the CI without at least documentation about it :) Thank you much for a very documented and detailed PR! |
|
Unfortunately it seems that the CLA between my company and google has expired -- it's been several years since I put in this PR and the person who signed it no longer works for my company 😢 . I put in with our legal team to redo it, fingers crossed they can do that soon. Sounds good about the issue, would you be able to file it? Or should I? I am not really that familiar with how that part of flatbuffers works. |
|
If you don't mind, I would appreciate it! Even just documenting its existence and maybe leaving a comment in the code for the moment would be fine -- just enough to document the conversation. |
|
Done! #8816 Even though the CLA is not signed yet, would you be able to authorize workflows so I can check if my CI fix worked? |
jtdavis777
added
the
waiting-for-update
|
hey @relativityspace-jsmith - Google pushed updates from their internal repo and this has resulted in merge conflicts here. |
- 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
72027a8 to
072b24b
Compare
|
OK this is rebased, and I also hopefully fixed the monster.bfbs issue thanks to @jtdavis777's help. Unfortunately still waiting to for our legal team to renew the CLA... |
3 participants
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!