dvidelabs / flatcc

FlatBuffers Compiler and Library in C for C
Apache License 2.0
632 stars 180 forks source link

#168: add cmake helper function to generate flatbuffer code #169

Closed wdobbe closed 8 months ago

wdobbe commented 3 years ago

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).

mikkelfj commented 3 years ago

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 commented 3 years ago

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

wdobbe commented 3 years ago

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.
mikkelfj commented 3 years ago

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.

wdobbe commented 3 years ago

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 commented 3 years ago

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.

mikkelfj commented 3 years ago

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 commented 3 years ago

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).

wdobbe commented 3 years ago

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.

mikkelfj commented 3 years ago

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.

wdobbe commented 3 years ago

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?

mikkelfj commented 3 years ago

@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.

madebr commented 3 years ago

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

mikkelfj commented 3 years ago

Should we close this PR then?

mikkelfj commented 8 months ago

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.