Fix flatbuffer builds for upcoming release #730
Conversation
pmai
added
the
Bug
pmai
force-pushed
the
fix/fix-flatbuffer-build
branch
from
46fab3e to
af2de19
Compare
pmai
added
the
ReadyForCCBReview
| @@ -1,3 +0,0 @@ | |||
| [submodule "flatbuffers"] | |||
There was a problem hiding this comment.
I am a bit surprised that the submodule is deleted here and find_package is used in CMakeLists instead. @pmai why do you suggest this particular change?
Background info: I find it very convenient to use a submodule here and not have the dependency on an (possibly) existing package. It is one of the pro-points on the list for flatbuffers that this is somehow possible..
There was a problem hiding this comment.
While I also found this somewhat convenient, it has its own problems (both in nailing down the version of flatbuffers needlessly, and not making it easy to configure for different target settings), and it is not what we do for protobuf. And with todays availability of vcpkg and others for installing flatbuffers, it seems better to stay consistently out of the submodule business for these kinds of dependencies.
There was a problem hiding this comment.
Understood. On the other side, nailing down the version of flatbuffers (and possibly protobuf) could prevent some missunderstandings in combining different models / tool plugins. The flexibility to configure for different target settings is indeed very important for our project. So if using submodules can be a problem to this point, I am ok with the change here.
| add_subdirectory("flatbuffers" | ||
| ${CMAKE_CURRENT_BINARY_DIR}/flatbuffers-build | ||
| EXCLUDE_FROM_ALL) | ||
| find_package(flatbuffers REQUIRED) |
There was a problem hiding this comment.
See my comment above.
| COMMAND $<TARGET_FILE:flatc> -I "${PROTOBUF_IMPORT_DIRS}" -o "${CMAKE_CURRENT_BINARY_DIR}" --proto "${CMAKE_CURRENT_SOURCE_DIR}/${proto}" | ||
| DEPENDS "${proto}" flatc | ||
| WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}" | ||
| COMMAND ${FLATBUFFERS_FLATC_EXECUTABLE} -I "${PROTOBUF_IMPORT_DIRS}" -I "${CMAKE_CURRENT_BINARY_DIR}" -o "${CMAKE_CURRENT_BINARY_DIR}" --proto "${proto}" |
There was a problem hiding this comment.
Is this change fixing an issue?
There was a problem hiding this comment.
Yes:
- Use of -I due to the cleaner separation of SOURCE and BINARY directories (we formerly generated files in the SOURCE directory, which we shouldn't)
- Since flatbuffers are not longer co-built, the flatc executable is not a target anymore, but rather referenced via variable.
osi_lane.proto
Outdated
There was a problem hiding this comment.
@pmai could you already test the changes and are they definetly fixing the problems when the flatbuffers compiler takes action?
There was a problem hiding this comment.
Yes, everything builds correctly and the correct enums are referenced. These were the only instances where multiple same-named types were present in the same file, so this should fix it across the board. However further review testing should be conducted.
|
CCB Review, 19.06.23: Can be merged. |
ThomasNaderBMW
added
ReadyToMerge
This fixes the flatc calls which were broken due to clearer separation of build and source directory usage. Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
This is a temporary work-around for a bug in the flatc compiler when translating protobuf idl to fbs idl, where definition after use will confuse the compiler when type names are reused in the same file but different surrounding types. Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
pmai
force-pushed
the
fix/fix-flatbuffer-build
branch
from
af2de19 to
ba5b394
Compare
This pull request collects all fixes to the flatbuffer build to make it fully functional: