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

wdobbe · 2020-12-16T12:32:04Z

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

madebr · 2020-12-16T14:44:31Z

cmake/FlatccGenerateSources.cmake

+        endif()
+
+        message(VERBOSE "Executing command ${FLATCC_COMPILER} ${FLATCC_CC_OPTIONS} -o ${FLATCC_GENERATED_SOURCE_DIRECTORY} ${FLATCC_DEFINITION_FILES}")
+        add_custom_command(OUTPUT ${FLATCC_EXPECTED_OUTPUT_FILES}


This needs DEPENDS for dependency tracking of the input flatbuffer files.

Without reading carefully and from memory, just adding DEPENDS will break Ninja build because CMake doesn't handle dependecies properly, even if it works with Make. If you look at some of the examples that build with CMAKE, these are rather clumsy because of this.

But without DEPENDS, the target sources don't get regenerated when the input sources get modified?

The add_custom_command documentation does mention ninja with multiple arguments, but not with DEPENDS btw.

I don't know because I am not a CMake expert. But I do know that I have spent several days trying to make it work (or something similar). Another who believed he could fix it had to admit he couldn't.

I'm looking into it now, I'll open a pr for installing flatcc-config.cmake.

This allows your users to use flatcc by doing find_package(flatcc REQUIRED) and call flatcc using flatcc::flatcc.

It looks like on every make, all tests are re-built.
This is because the dependency tree is not correct.
I'll look into that too.

@wdobbe
You can then re-use/rebase your FlatccGenerateSources.cmake script on my pr.

OK, here is an old discussion about broken dependencies - I haven't reread it, but linked here for reference:

#24 (comment)

I'm looking into it now, I'll open a pr for installing flatcc-config.cmake.

This allows your users to use flatcc by doing find_package(flatcc REQUIRED) and call flatcc using flatcc::flatcc.

Yes, that is a good idea. I have plenty examples of how to generate the config files (correctly) so if you want me to do it, let me know.

madebr · 2020-12-16T14:49:18Z

cmake/FlatccGenerateSources.cmake

+set(FLATCC_BIN_PATH "$ENV{FLATCC_BUILD_BIN_PATH}")
+if (FLATCC_BIN_PATH)
+    #user provided location where asn1c compiler executable is installed
+    find_program(FLATCC_COMPILER flatcc
+                 PATHS ${FLATCC_BIN_PATH}
+                 NO_DEFAULT_PATH
+                 NO_SYSTEM_ENVIRONMENT_PATH
+                 NO_CMAKE_SYSTEM_PATH
+)
+else()
+    #Find compiler exe
+    find_program(FLATCC_COMPILER flatcc)
+endif()


I think it's ok to do this with one find_program:

it's possible to override by doing -DFLATCC_COMPILER=xxx.

Using -DFLATCC_ROOT, you can set a prefix of flatxx

Using the FLATCC_ROOT environment variable, you can also set a prefix

I think the options NO_DEFAULT_PATH, NO_SYSTEM_ENVIRONMENT_PATH and NO_CMAKE_SYSTEM_PATH are not needed.

Suggested change

set(FLATCC_BIN_PATH "$ENV{FLATCC_BUILD_BIN_PATH}")

if (FLATCC_BIN_PATH)

#user provided location where asn1c compiler executable is installed

find_program(FLATCC_COMPILER flatcc

PATHS ${FLATCC_BIN_PATH}

NO_DEFAULT_PATH

NO_SYSTEM_ENVIRONMENT_PATH

NO_CMAKE_SYSTEM_PATH

)

else()

#Find compiler exe

find_program(FLATCC_COMPILER flatcc)

endif()

#user provided location where asn1c compiler executable is installed

find_program(FLATCC_COMPILER flatcc

PATHS ${FLATCC_ROOT} ENV FLATCC_ROOT

PATH_SUFFIXES bin

)

#user provided location where asn1c compiler executable is installed

asn1c?

Sorry, copy/paste error from a similar script for the ASN1C compiler.
I'll correct that.

madebr · 2020-12-16T14:51:33Z

cmake/FlatccGenerateSources.cmake

@@ -0,0 +1,86 @@
+ cmake_minimum_required(VERSION 3.5)


I would split this file in 2:

Export the flatcc target as flatcc::flatcc with install(TARGETS) and install(EXPORT).

Inside flatcc-config.cmake, do include("${CMAKE_CURRENT_LIST_DIR}/FlatccGenerateSources.cmake") which then only contains the function. Instead of the FLATCC_COMPILER, it can then use the imported flatcc::flatcc executable target.

Yes, that would be a better solution.

mikkelfj · 2020-12-16T15:07:43Z

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

Regarding minimum required version, there is reason why flatcc uses cmake_minimum_required (VERSION 2.8)

Otherwise it will not build on some older conservative platforms. If it is really necessary to use a more recent version and it only affects users own programs opting in to this feature then maybe OK, but otherwise it would interfere with portability which is important to FlatCC.

mikkelfj · 2020-12-16T15:09:41Z

README.md

+
+Optionally you can let cmake know the directory where the flatcc executable
+is located in environment variable `FLATCC_BUILD_BIN_PATH`. This is especially
+usefull when cross-compiling. In that case you should provide the directory


Regarding minimum required version, there is reason why flatcc uses cmake_minimum_required (VERSION 2.8)

Otherwise it will not build on some older conservative platforms. If it is really necessary to use a more recent version and it only affects users own programs opting in to this feature then maybe OK, but otherwise it would interfere with portability which is important to FlatCC.

Edit: sorry was confused with PRs from Conan.
~~I had to update the cmake_minimum_required version from 2.8 to 3.1 in another recipe PR today because a conan-center hook rejects the PR if the cmake_minimum_required is not at least 3.1.~~

In this case the minimum cmake version is 3.5 because from that version cmake function cmake_parse_arguments checks for value arguments that are miss their value, so it makes the function more fool-proof.

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.
The only thing to do is that you have to do to make the module work is add the FlatCC cmake subdir to your CMAKE_MODULE_PATH.

Edit: when we also export a flatcc cmake config file as suggested by @madebr then appending CMAKE_MODULE_PATH is no longer necessary.

But if it works the same for correct arguments on earlier versions then an earlier MIN version will still support the check for users with a recent CMake while not breaking the build for those who do not.

mikkelfj · 2020-12-16T15:32:53Z

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

wdobbe · 2020-12-16T16:36:43Z

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.

The module is optional, the end-user can choose whether to use it or not.
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 · 2020-12-16T16:42:47Z

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 · 2020-12-16T17:00:40Z

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 · 2020-12-16T17:03:49Z

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 · 2020-12-16T17:09:49Z

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 · 2020-12-16T17:12:06Z

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 · 2020-12-16T17:22:51Z

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 · 2020-12-16T17:26:31Z

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 · 2020-12-16T17:35:12Z

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 · 2020-12-17T08:12:47Z

@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 · 2020-12-17T14:37:57Z

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

mikkelfj · 2020-12-18T08:47:13Z

Should we close this PR then?

mikkelfj · 2023-10-24T18:15:29Z

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.

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

9ed0475

wdobbe mentioned this pull request Dec 16, 2020

#3907: add component info in conanfile.py, add cmake helper module, e… conan-io/conan-center-index#3908

Closed

4 tasks

madebr reviewed Dec 16, 2020

View reviewed changes

mikkelfj reviewed Dec 16, 2020

View reviewed changes

madebr mentioned this pull request Dec 16, 2020

cmake: flatcc_generate_sources + add_custom_command + crossbuild + ci #171

Closed

mikkelfj mentioned this pull request Oct 25, 2021

Newer CMake is going to drop <2.8.12 #217

Closed

mikkelfj force-pushed the master branch 4 times, most recently from a8e7a8a to d7f50b2 Compare November 1, 2022 10:48

mikkelfj mentioned this pull request Dec 9, 2022

[cmake:]fix the dependence to avoid generate files every time, even though no changed #258

Open

mikkelfj closed this Oct 24, 2023

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

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

Conversation

wdobbe commented Dec 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikkelfj commented Dec 16, 2020

mikkelfj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wdobbe Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

wdobbe Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikkelfj commented Dec 16, 2020

wdobbe commented Dec 16, 2020 • edited Loading

mikkelfj commented Dec 16, 2020

wdobbe commented Dec 16, 2020 • edited Loading

wdobbe commented Dec 16, 2020

mikkelfj commented Dec 16, 2020

mikkelfj commented Dec 16, 2020

wdobbe commented Dec 16, 2020

mikkelfj commented Dec 16, 2020

wdobbe commented Dec 16, 2020

mikkelfj commented Dec 17, 2020 • edited Loading

madebr commented Dec 17, 2020

mikkelfj commented Dec 18, 2020

mikkelfj commented Oct 24, 2023

wdobbe Dec 16, 2020 •

edited

Loading

wdobbe Dec 16, 2020 •

edited

Loading

wdobbe commented Dec 16, 2020 •

edited

Loading

wdobbe commented Dec 16, 2020 •

edited

Loading

mikkelfj commented Dec 17, 2020 •

edited

Loading