cmake: flatcc_generate_sources + add_custom_command + crossbuild + ci

[madebr]

This pr does the following:

• use add_custom_command to generate the sources. Add all generated sources literally to OUTPUTS. The key to don't have sources recompile is to add the generated sources to add_executable. The flatcc maybe has an argument to return what files are generated, but this is work for #169. flatcc should have an option that returns the generated sources (and nothing else). e.g.:

  flatcc -a monster.fbs
  flatbuffers_common_builder.h flatbuffers_common_reader.h monster_builder.h monster_reader.h monster_verifier.h

  This information + flatcc -d monster.fbs allows to know the dependant sources + generated sources.

• Create aliases (flatcc::runtime, flatcc::libflatcc, flatcc::cli) for the 2 libraries + executable, and use it everywhere. make install will also create these 3 targets. This means that the script by #169 can be used by flatcc itself + users of flatcc (by using imported targets). So flatcc can be used as:

  find_package(flatcc REQUIRED)
  add_custom_command(OUTPUTS genrated.h
      COMMAND flatcc::cli input.fbs ...)
  add_executable(myexe source.c generated.h)
  target_link_libraries(myexe PRIVATE flatcc::runtime)

I have tested it on cmake 2.8.12.1 (with gmake and ninja).

[mikkelfj]

Thanks. It sounds a bit of a hack to add generated files to executables, but if it works ... Note that Windows builds are important, and Appveyor just failed.

Do you really need to all output files of flatcc all these places? I worked with a meson build where I just used the most important output file as a dependency because Ninja does not support multiple output files (there is an issue on it).

Note that flatcc supports generating gnu make style .dep files, but it probably isn't helpful here. I used it with the meson build where it worked fine.

On discussion in related pr on build dependencies. things are coming slowly back to me. I think the benchmarks tests cannot be built with CMake as it is, though I do not recall why.

https://github.com/dvidelabs/flatcc/tree/master/test/benchmark

They are building on the old meson branch I once made: https://github.com/dvidelabs/flatcc/blob/meson/test/benchmark/benchflatcc/meson.build

[mikkelfj]

Can you please explain exactly what the problem is that you try solve:

You mentioned in other PR that tests were always building. But when I use ninja build incrementally, nothing gets build that isn't out of data, at least that I have noticed. Maybe this differ for Make?

[madebr]

On current master, when you do:

cmake .. -DFLATCC_TEST=ON -DFLATC_INSTALL=ON
make
make install

The test executables are rebuilt on every make invocation.

This is because flatcc is re-run every time. CMake detects that the dependencies of the tests (the includes) are newer and will rebuild the sources + executables.

I didn't test the ninja behavior of current master. Because ninja runs the compilation on one line, the rebuilds might be hidden for you? What does ninja -v give?

I'm currently looking into the Windows failure,. I can't reproduce it on my Windows box.

[mikkelfj]

Right, I see that now. ninja -v -j1

[mikkelfj]

You may also want to look into ci-more branch, especially on Windows to ensure that MSVC 2010 is supported.

[madebr]

The error on appveyor was due to the interaction between a parallel build and cmake's add_custom_command.

On cmake, add_custom_command does not create a target (=.vcxproj project). This causes the copy + flatcc to be called in parallel, which causes errors on Windows. This only happens with MSbuild..

I have added a custom target in the json tests because that one is the only location where the sources generated by one add_custom_command are used by multiple add_executable targets.

This json test approach can be adopted by @wdobbe 's cmake script . So all add_custom_command's I have added should get replaced by his flatcc_generate_sources.

I also added a comment how this can be simplified if the cmake minimum version would get bumped to 3.1. I have not verified this approach though.

Also note that cmake 3.19.1 emits the following warning on your cmake script:

  Compatibility with CMake < 2.8.12 will be removed from a future version of
  CMake.

  Update the VERSION argument <min> value or use a ...<max> suffix to tell
  CMake that the project does not need compatibility with older versions.

[madebr]

@mikkelfj

flatcc has a -d option to list all dependencies. Does it have an option to list all generated sources?

[mikkelfj]

flatcc has a -d option to list all dependencies. Does it have an option to list all generated sources?

No not that I can recall.

[mikkelfj]

Well, the dependency file is actually a source to dependency list, but I think it is constrained because of Ninja. It's been a while. Like this:

EDIT : WRONG ouput1 output2 : src1 src2 src3

or

ouput1 : src1 src2 src3 ouput2 : src1 src2 src3

EDIT: RIGHT src1 src2 : dep1 dep2 dep3

So no that does not give the output.

[madebr]

@mikkelfj I thought about it a bit further, and it isn't useful for flatcc itself, because flatcc is not available while configuring cmake.

It is useful though for consumers of flatcc. Using the following dependency information,

generated_header1.h generated_header2.h: source.fbs
source.fbs: included1.fbs  included2.fbs

cmake can automatically deduce the dependency information.

[mikkelfj]

Yes but it would not be good for the intended purpose because these are separate build steps, especially considering how ninja works and how build generators works towards it.

It would probably need to be a different switch.

The build model works like multiple sources to a single target in the build graph. Technically there can be more output files, but the model treats them all as one. A single output file is enough to represent all of them. Theoretically the build could break halfway and produce an incomplete result, but ninja cannot handle multiple files currently.

Another option would be to ask flatcc to produce a flag file once it has successfully written all output files. But can also do that in a wrapper command: flatcc ARGS && touch flagfile

[madebr]

I'm on Windows now, but I tested it with cmake .. -GNinja and it worked fine. Even with adding multiple OUTPUTS to add_custom_command. Perhaps a more recent ninja fixed this problem you're referring to?

Please also note that I have enabled test/cgen_test, which had a note about it not working with ninja.

[mikkelfj]

Take a look at the meson build rules on the meson branch:

https://github.com/dvidelabs/flatcc/blob/meson/rules/meson.build

This build will detect if you make a change in a the third level include file and build a file that depends on any generated output, provided that file has set the fbs file as a dependency.

[mikkelfj]

Please also note that I have enabled test/cgen_test, which had a note about it not working with ninja.

Ahh, that was the one - it was four years ago ... thanks!

[mikkelfj]

Ninja should have fixed this now: https://github.com/ninja-build/ninja/issues/1184

[mikkelfj]

I don't really have time to try understand the implications of this right now, but if flatcc does not already support the improved dep file format, I'll certainly be adding it at some point.

[mikkelfj]

https://github.com/dvidelabs/flatcc/blob/ce931d0e34b9a4a9dd69939f2d5860c781b42d8b/src/compiler/flatcc.c#L209

[madebr]

Ah, I misread the monster example. I didn't see that it had all 3 fbs files as input and thought that it generates a lot of outputs depending on what it includes. So it is indeed possible to pre-calculate the expected outputs like you do in the meson branch. I think this will be the easiest approach instead of adding a new flatcc option.

But for the dependency information on other fbs sources, that is not fixed by this.

@wdobbe I will take a go at it..

[mikkelfj]

I'm not sure that I follow - where are the three input files listed? link?

flatcc has the -r (--recursive) option to generate everything included, or just the file listed.

[madebr]

I was hinting at test/json_test/CMakeLists.txt and test/monster_test/CMakeLists.txt.. It has the main monster_test.fbs + all other included fbs sources listed in DEPENDS.

Perhaps I should create a cmake script that applies a regex to the fbs sources recursively to grep all dependencies. This will make it work for flatcc itself + its users.

[mikkelfj]

DEPENDS flatcc_cli "${FBS_DIR}/monster_test.fbs" "${FBS_DIR}/include_test1.fbs" "${FBS_DIR}/include_test2.fbs"

Ah right. This is something I'd really like to avoid because in real life large projects this can become difficult to manage when schemas are modified and there are many included schema files. But you gotta do what you gotta do.

I'm not too keen on hacking a new option in flatcc either, but if it can work with a correct and improved depfile generation, I think that is the way to go. Only I'm not going to fix that right away. It also needs a backwards compatibility mode I suspect.

[madebr]

@wdobbe I used your script as a base to generate the headers for the tests/samples. The script is also installed.

@mikkelfj The output files and dependencies are extracted from the files using some regex'es. Because travis-ci is stuck on Macos, I also included a github actions workflow that can replace appveyor and travis CI. See https://github.com/madebr/flatcc/actions/runs/427450742 When you're happy with the big lines of this pr, I'll remove it from this pr and open a new one, dedicated to it.

[mikkelfj]

The output files and dependencies are extracted from the files using some regex'es.

From which files? The dep files?

Because travis-ci is stuck on Macos, I also included a github actions workflow that can replace appveyor and travis CI.

Much appreciated. I presume you have seen the new issue I raised: https://github.com/dvidelabs/flatcc/issues/170 Note that before I can switch from Travis/Appveyor it is important to support old compilers on Windows and Linux - but on a separate branch - it is too slow for master, at least on Travis / Appveyor. On GH Actions you can can make a branch test and use the same action script on multiple branches - in the current setup I use different scripts on ci-more.

[mikkelfj]

Note deprec warning on CMake on MacOS build:

CMake Deprecation Warning at CMakeLists.txt:5 (cmake_minimum_required): Compatibility with CMake < 2.8.12 will be removed from a future version of CMake.

@wdobbe @madebr
As to CMake version, I think the CMake version supported on Ubuntu 18.04 should suffice. If I get this right that would be 3.10: https://packages.ubuntu.com/bionic/cmake

[wdobbe]

By the way @madebr : thanks a lot for all the work you put into this. I really appreciate it.

[madebr]

Note deprec warning on CMake on MacOS build:

CMake Deprecation Warning at CMakeLists.txt:5 (cmake_minimum_required): Compatibility with CMake < 2.8.12 will be removed from a future version of CMake.

@wdobbe @madebr As to CMake version, I think the CMake version supported on Ubuntu 18.04 should suffice. If I get this right that would be 3.10: https://packages.ubuntu.com/bionic/cmake

It would be nice to bump to at least 3.1. I think 3.1 suffices, but I didn't test it though.

This would allow the following usage:

find_package(flatcc REQUIRED)

include(FlatccGenerateSources)
flatcc_generate_sources(flatbuffer_headers DEFINITION_FILES input.fbs ALL)

add_executable(main main.cc)
target_link_libraries(main PRIVATE flatcc::runtime flatbuffer_headers)

Currently, the following is required, which is awkward (imho).

find_package(flatcc REQUIRED)

include(FlatccGenerateSources)
flatcc_generate_sources(DEFINITION_FILES input.fbs ALL TARGET flatbuffer_headers)

add_executable(main main.cc)
target_link_libraries(main PRIVATE flatcc::runtime)
add_dependencies(main flatbuffer_headers)

[madebr]

Using flatcc::cli will fail I think when cross-compiling, because that target will then point to the host arch flatcc exe while we need the build arch flatcc exe. I think it is unavoidable that the caller needs to provide the location of the build arch flatcc exe, preferably via an environment variable. In the Conan recipe for flatcc I already export such a variable when cross-compiling is detected. That variable is then automatically picked up be the flatcc_generate_sources function.

@wdobbe I'm thinking about splitting up the compiler and runtime component. Then you can specify the search path of the runtime/library. See the documentation of find_package. Using <PackageName>_ROOT, you can then override the location of the compiler.

I'm envisioning the following usage

# Only import the runtime (flatcc::runtime and flatcc::libflatcc)
find_package(flatcc REQUIRED COMPONENTS runtime)
# Only import the compiler (flatcc::cli)
find_package(flatcc REQUIRED COMPONENTS compiler)

Internally, Findflatcc.cmake will do find_package(flatccRuntime REQUIRED) and find_package(flatccCli REQUIRED). So by settings flatccCli_ROOT to the root of a build prefix, and flatccRuntime_ROOT to the root of a host prefix, cross building is supported. And the target flatcc::cli reamains available.

@mikkelfj By the way, are you happy with the names of the targets flatcc::cli, flatcc::runtime and flatcc::libflatcc? These targets will be exposed to flatbuffer users.

[mikkelfj]

By the way, are you happy with the names of the targets flatcc::cli, flatcc::runtime and flatcc::libflatcc? These targets will be exposed to flatbuffer users.

I am not familiar with CMake namespaces / modules so I can't really say. But the final output products shouldn't change because that would break things.

I am a bit worried about libflatcc because already many users confuse libflatccrt with libflatcc and even if runtime is runtime, people tend to look for the word lib. So maybe flatcc::libflatccruntime ?

[madebr]

Namespaces are just targets with :: that are aliases of other targets or are imported.

flatcc::libflatccruntime is a bit too much, imho. How about the following? flatcc::cli, flatcc::runtime and flatcc::libcli?

The outputs won't change. This is just about picking names to minimize confusion.

[mikkelfj]

How about flatcc:cli flatcc::compiler flatcc::runtime ? This matches the src directory layout.

[madebr]

I was confused by cli vs compiler, wouldn't people be confused as well? But I guess it's ok. Ideally, people should only use the new flatcc_generate_sources function + flatcc::runtime.

[mikkelfj]

Probably. But I don't really see a solution that doesn't confuse.

[mikkelfj]

Ideally, people should only use the new flatcc_generate_sources function + flatcc::runtime.

I wouldn't be using cmake, but in one of my own projects I use libflatcc to compile schema files into binary schema files which I then use to generate source code for other protocols.

[wdobbe]

I'm envisioning the following usage

# Only import the runtime (flatcc::runtime and flatcc::libflatcc)
find_package(flatcc REQUIRED COMPONENTS runtime)
# Only import the compiler (flatcc::cli)
find_package(flatcc REQUIRED COMPONENTS compiler)

Internally, Findflatcc.cmake will do find_package(flatccRuntime REQUIRED) and find_package(flatccCli REQUIRED). So by settings flatccCli_ROOT to the root of a build prefix, and flatccRuntime_ROOT to the root of a host prefix, cross building is supported. And the target flatcc::cli reamains available.

If that works its fine with me, but can we make that work as well within Conan when using the cmake_find_package generator? Because I suppose the flatcc config files will have to be removed in the Conan package when submitting to Conan-center-index.

[madebr]

@wdobbe

If that works its fine with me, but can we make that work as well within Conan when using the cmake_find_package generator? Because I suppose the flatcc config files will have to be removed in the Conan package when submitting to Conan-center-index.

It is compatible with conan with minimal patching (at least, until conan can create IMPORTED executable targets). Please test the project in test/install_folder with your cross build toolchain. I have written a little text in README about how to override the search paths.

@mikkelfj As I mentioned earlier, the minimum required cmake version for the flatcc_generate_sources function is 3.3. I have also added a test (test/install_test) that tests an installed flatcc prefix. See the last step of the github actions script.

[mikkelfj]

Looking at the monster sampe, there is FBS_DIR. But is not used directly and I don't think it is used by the generator. So I suggest either using it explicitly in front of monster.fbs, or removing it. Same apply to other places where this might be the case. If FBS is needed it should be documented in README.

[EDIT: re-reading monster sample it does seem that FBS_DIR is used and thus OK].

I didn't look closely, but I think the README section belongs under build section that is placed later. If more prominent position is needed in README for this or other build getting started doc, it should probably be a short reference to the relevant section. Open to suggestions.

[wdobbe]

I sent PR madebr/flatcc#1 to @made_br's cmake_dep branch mostly to add a few more input validation checks in the flatcc_generate_sources function.

[mikkelfj]

On the syntax for generate_sources:

DEFINITION_FILES: suggest changing to SCHEMA_FILES.

And I'm not really sure what the current SCHEMA does but I think it is binary schema, in which case BINARY_SCHEMA or REFLECTION probably would be better. I'm not claiming the flatcc cli options necessarily make a lot of sense. Suggestions are welcome.

[mikkelfj]

If there is an enhanced GH-actions ci-more build, the regular ci.yml should probably have a branch filter so it doesn't also run on ci-more.

[madebr]

If there is an enhanced GH-actions ci-more build, the regular ci.yml should probably have a branch filter so it doesn't also run on ci-more.

I've added the filter, although I don't think it is really needed. The CI test is really short and fast.

[madebr]

On the syntax for generate_sources:

DEFINITION_FILES: suggest changing to SCHEMA_FILES.

And I'm not really sure what the current SCHEMA does but I think it is binary schema, in which case BINARY_SCHEMA or REFLECTION probably would be better. I'm not claiming the flatcc cli options necessarily make a lot of sense. Suggestions are welcome.

I've made this change.

Please check all options of the function, because once added (and released), it's very hard (not to say impossible) to remove them.

[mikkelfj]

BTW: ALL should imply recursive. flatcc cli documents -a as : -cwvr

[mikkelfj]

Maybe it is right to use READER as default since flatcc documents this to be the default. It is easy to add ALL if that is needed.

[madebr]

Maybe it is right to use READER as default since flatcc documents this to be the default. It is easy to add ALL if that is needed.

My idea was to keep the differences in behavior between the function and flatcc as small as possible.

[mikkelfj]

@derosier Do you have any comments to the updated cmake setup?

[mikkelfj]

I'm not too happy about introducing a python dependency into the build.

[madebr]

@mikkelfj

The current cmake script supports reference files. With make regenerate_reference_files, you can regenerate the reference files. They are stored in test/reference. The outputs of the tests are compared when running ctest.

This comparison is diabled for:

• cgen_test because it tests compiling a source
• load_test because its output contains variable data (bandwidth, throughput, time

It also uncovers a problem: the output of monster_test is not reproducible. See https://github.com/madebr/flatcc/actions/runs/434787563 for the output.

[madebr]

I'm not too happy about introducing a python dependency into the build.

It's a fallback for when bash is missing.

[madebr]

Indeed, this pr started as getting the dependencies in order calling flatcc in-build, and not it's also adding reference files to the test system. I'll remove the last commits, and open an issue.

[mikkelfj]

It's a fallback for when bash is missing.

We should try to avoid such dependencies. There are driver scripts now, but you are not forced to use them, except the benchmarks, and that isn't ideal either.

Where diff is needed, there is an equivalent for Windows using .bat files which I have used elsewhere, though I'm not convinced it is necessary to do that here.