flatcc icon indicating copy to clipboard operation
flatcc copied to clipboard

#168: add cmake helper function to generate flatbuffer code

Open wdobbe opened this issue 5 years ago • 14 comments

Resolves #168 .

Install a cmake module that provides cmake function flatcc_generate_sources. This function simplifies generation of C code from flatbuffer definition file(s).

wdobbe avatar Dec 16 '20 12:12 wdobbe

Can you provide an example within FlatCC's test cases that uses this module?

I'm not convinced I want to accept this PR because I'd like to keep CMake maintenance to a minimum. It is has been rather painful.

mikkelfj avatar Dec 16 '20 15:12 mikkelfj

BTW: any idea why Travis build breaks on MacOS? Is it just Travis that hasn't upgraded its platform properly?

mikkelfj avatar Dec 16 '20 15:12 mikkelfj

Can you provide an example within FlatCC's test cases that uses this module?

This is how I call it in one of our company cmake files:

flatcc_generate_sources(GENERATED_SOURCE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/datadef
                        GENERATE_BUILDER
                        GENERATE_VERIFIER
                        EXPECTED_OUTPUT_FILES datadef/seclif_protocol_reader.h 
                                              datadef/seclif_protocol_builder.h 
                                              datadef/seclif_protocol_verifier.h
                        DEFINITION_FILES ${CMAKE_CURRENT_SOURCE_DIR}/datadef/seclif_protocol.fbs
)

I'm not convinced I want to accept this PR because I'd like to keep CMake maintenance to a minimum. It is has been rather painful.

  1. The module is optional, the end-user can choose whether to use it or not.
  2. I don't see the CMake maintenance problem. CMake has always been completely backwards-compatible, so once the function works it shouldn't require any maintenance.

wdobbe avatar Dec 16 '20 16:12 wdobbe

I'm suggesting to add an example of using this module within the FlatCC project, but perhaps not replacing existing since it could break portability with CMake version. I'm not sure if the module only works after CMake install and the README should probably also clarify how to make it available.

mikkelfj avatar Dec 16 '20 16:12 mikkelfj

BTW: any idea why Travis build breaks on MacOS? Is it just Travis that hasn't upgraded its platform properly?

When adding flatcc to conan-center-index we also had problems on MacOS because of a new security feature called Security Integrity Protection (SIP). That could be the case, however I currently don't see build logs for MacOS for this PR? And the only Mac I currently have access to (due to Covid-19 lockdown) is too old to have SIP.

wdobbe avatar Dec 16 '20 17:12 wdobbe

I'm suggesting to add an example of using this module within the FlatCC project, but perhaps not replacing existing since it could break portability with CMake version. I'm not sure if the module only works after CMake install and the README should probably also clarify how to make it available.

Ok, I will do that once changes suggested by you and @madebr are implemented.

wdobbe avatar Dec 16 '20 17:12 wdobbe

I'm developing flatcc on Mac Intel with Ninja. Travis has been building without issues before, except that it is often very slow. I think all travis builds use Make. It is unlikely that this PR triggered a build issue, but it is the first time that I see it. I'm inclined to move everything (Windows, Mac, Linux) to GH Actions, but its not happening right away.

mikkelfj avatar Dec 16 '20 17:12 mikkelfj

BTW: conan is different because it uses a binary dist, I think. Building from source should only give SIP issues if the core tools are broken (homebrew?, clang, make, cmake).

mikkelfj avatar Dec 16 '20 17:12 mikkelfj

This is the comment I added for the workaround at that time (see here and here):

#On MacOS System Integrity Protection (SIP) will clear the DYLD_LIBRARY_PATH variable.
#As a result calling flatcc from cmake will currently not work if the flatcc executable
# is linked shared. As a workaround we generate the flatbuffer C files in the Conan recipe
# when on MacOS and flatcc option 'shared' is True.

So the issue I talked about only arises when the flatcc executable links dynamically to the flatcc library and flatcc is called from cmake.

wdobbe avatar Dec 16 '20 17:12 wdobbe

DYLD, OK.

Build logs - follow details in the "Some checks were not successful" https://travis-ci.org/github/dvidelabs/flatcc/jobs/750009859 The problem is homebrew trying to install ninja, it seems. Nothing that I have changed in years.

mikkelfj avatar Dec 16 '20 17:12 mikkelfj

I'm not too familiar with homebrew (I use conan nowadays) however in the build logs I don't see anything go particularly wrong, apart from the timeout at the end. Maybe the homebrew install was stuck or too slow?

wdobbe avatar Dec 16 '20 17:12 wdobbe

@wdobbe I don't have the full overview here, but it seems that @madebr has already integrated most or all of your changes into his PR #171 If that PR passes review I'd probably squash it into master as a single commit. In that case you might not get listed as a contributor, so you should probably make a commit to that PR. But as I said, I don't have the full overview here.

mikkelfj avatar Dec 17 '20 08:12 mikkelfj

I have used @wdobbe 's commit and kept his name, so he should be mentioned as co-creator.

madebr avatar Dec 17 '20 14:12 madebr

Should we close this PR then?

mikkelfj avatar Dec 18 '20 08:12 mikkelfj

Sorry for not following up on this. We have several build PRs going and I need to narrow this down. If you are still interested, please comment on PR #258. The plan is to make a release soon, then introduce changes to the build.

mikkelfj avatar Oct 24 '23 18:10 mikkelfj