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 #168: add cmake helper function to generate flatbuffer code #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 #168: add cmake helper function to generate flatbuffer code #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:
ninja-build/ninja#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]

flatcc/src/compiler/flatcc.c

Line 209 in ce931d0

static int __flatcc_gen_depends_file(fb_parser_t *P)

[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: #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.

[madebr]

I rebased on top of master.

@mikkelfj
To avoid bitrot, can you please take a(nother) look at this?

[mikkelfj]

Is it ready? I thought you had abandoned it.

[madebr]

Is it ready? I thought you had abandoned it.

I think so, I stopped pushing changes because I ran out of ideas 😄
I'm looking at the travis error atm.

[mikkelfj]

OK, I'll have a look at some point. But I'll need to make a release before merging. Looking forward to CI fixes.

[madebr]

I messed up my latest force push, but was able to recover using c723a5e.

[mikkelfj]

Sorry for not following up on this - we know have a functioning CI build again via #250. As discussed in #217, #217 (comment) this PR is probably doing too much at a time.

A new PR #258 has been made. It also addresses the rebuild too much issue.

@madebr and @midokura-xavi92 would mind having a look at this PR? It seems to be a fairly simple change, although it does depend on all output of a directory.

[mikkelfj]

I finally got some time to work on flatcc. I'm going to close this PR because we have several build updates going on including PR #258. You are still welcome to comment on that PR and introduce your ideas, but I can't have two major PR on the build system. The plan is make a release soon, before major updates to the build system.