Integrate Meson as optional build system

[amcn]

What does this PR do?

it updates the existing meson branch against master making various changes here and there to get it to match the existing system. It's now possible to build, run the tests, and the benchmarks under meson using the following commands:

$ meson setup build
$ meson test -C build
$ meson test -C build --benchmark

In addition, the shell scripts in the scripts directory have been updated to allow one to specify Meson as the build system alongside the existing build configurations using CMake. This required some changes to a number of scripts and configurations. Careful review of this would be appreciated, as I am unsure if my changes are acceptable.

A job has been added to the CI workflow which builds, runs the tests, and the benchmarks under Linux, OSX, and Windows. This job has been set to not block the overall workflow if it fails.

While care has been taken to match the existing behaviour with that of CMake, I am sure that I have missed some things. There are probably also old things from the previous Meson branch that are now not useful.

Motivation

This solves a use case I have: using flatcc as a subproject of a system that is built with meson. With these changes in master (and a small subsequent push to meson's wrapdb) it should be possible for users of meson to get access to flatcc by doing the following:

$ meson wrap install flatcc

and then using it directly in their build definitions:

flatcc = subproject('flatcc')
flatcc_gen = flatcc.get_variable('flatcc_gen')

output_c_files = flatcc_gen.process('my_flatbuffer.fbs')

I welcome your feedback. This aims to respond to #56 .

[mikkelfj]

Thank you very much, I'll review this later, but a few quick observations:

• I don't see why build.cfg.ninja should be removed, it is used with CMake ninja builds.
• I assume you have some legit compiler warnings to get rid of, however, I'd prefer to avoid somewhat unrelated changes to the source code in the same PR, or at least the same commit. Otherwise I'm happy with a single commit since I tend to squash PR's anyway.

There are also changes to other build scripts, possibly a refactoring, I haven't looked closely. But essentially, the changes might be fine, but they should apply independently of whether meson is included or not.

[mikkelfj]

Also, I noted that you updated Appveyor but it is OK to focus on GH Actions. I forgot to mention that we will eventually probably replace Appveyor fully with GH Actions, but currently it builds more windows and MSVC versions that the GH actions build does (contribs welcome on that front).

[amcn]

• I don't see why build.cfg.ninja should be removed, it is used with CMake ninja builds.

build.cfg.ninja was renamed to build.cfg.cmake and its contents were updated so that the shell scripts continue to function as before. Basically:

$ ./scripts/initbuild.sh # this loads build.cfg.cmake (and hence ninja) by default
$ ./scripts/initbuild.sh meson # this loads build.cfg.meson
$ ./scripts/initbuild.sh make-* # this loads build.cfg.make-*

So users who currently use ./scripts/initbuild.sh get the same behaviour as before.

• I assume you have some legit compiler warnings to get rid of, however, I'd prefer to avoid somewhat unrelated changes to the source code in the same PR, or at least the same commit. Otherwise I'm happy with a single commit since I tend to squash PR's anyway.

So there are two source changes:

1. In bcc1c3e: compiling under Meson with -Db_sanitize=undefined raised some warnings. Most of these were warnings about misaligned loads (originating from cmetrohash's unaligned.h) which I disregarded as the intended behaviour. The remaining warning was regarding a case were a NULL was passed as the copy source to memcpy. I added a check here to fix this, and reran the tests without any regressions. The change seems benign to me, but you are the judge of that.

2. In a32fc9f: The benchmark code is now compiled with the same flags as the runtime and library. Up until now this code had escaped the strictness of these flags, and so in order to satisfy the compiler (particularly Clang on OSX) I modified the benchmark code to fix these warnings. They almost exclusively related to implicit type conversions between differently-sized types.

Please let me know how you feel about these. I can rework or remove them if that suits better.

There are also changes to other build scripts, possibly a refactoring, I haven't looked closely. But essentially, the changes might be fine, but they should apply independently of whether meson is included or not.

Indeed, there was quite a bit of refactoring to accommodate the inclusion the Meson configuration. I tried to make it so that the existing behaviour is maintained for current users and that only those who opt-in explicitly to meson will build with meson, and so my changes were implemented with that in mind.

Many thanks, and I look forward to your feedback.

[amcn]

Any updates on this?

[mikkelfj]

Sorry, glad you are asking. I'm a bit busy right now, but I have definitely not forgotten about it.

[mikkelfj]

@amcn heads up - I'm starting to do the review, if you are still willing to follow up on this.
I'm sorry I have not had the time to look into this before now.

Overall I think it is great work, I'm just focusing on the details to get it into a mature and polished state.

I will not review all the individual meson.build files. I trust they are OK if tests pass, and they seem to do that at least on my machine (MacOS Sonoma M2 chip).

as to the checks on this PR:
I don't understand why only the appveyor build runs - I'd really like to see that github actions build execute. I'm not sure why that doesn't happen, or am I missing something. Notably I'm concerned about some of the warnings you fix and how that affects old systems on the cmake build.
We can also remove the appveyor build altogether if we get the GH actions build running as we are going to phase out appveyour anyway.

the windows meson build commit has lines like these:

if buildtype == 'debug' and build_machine.system() != 'windows'
  flatccrt_name = 'flatccrt_d'
  flatcc_name = 'flatcc_d'
endif

I'm not sure this the is best option. If you build with clang or mingw you probably want the same conventions as the other targets (I'm not sure, but I think so, haven't checked what CMake build does), so you are likely looking for a MSVC check.

On your above comments:

"... mostly warnings ...
originating from cmetrohash's unaligned.h"
I couldn't find any changes to metrohash, maybe I fixed that separately earlier on, I think we discussed this before. In any case I'd really like changes unrelated to meson port to be separate commit, preferably a separate PR. I can also just make the changes directly on main if you prefer. I see you have made some effort to isolate on separate commit for UB fix, but there are other changes to benchmark and test code in several other commits.

"The benchmark code is now compiled with the same flags"
wrt. warnings etc. this is fine, but I'm not sure it is good to use the same flags in general because benchmarking is really really hard to do right. One blink, and the optimizer removes entire code sections instead of measuring them. I really don't recall what I did back then, only that it required some tuning, especially in the driver source code.

A few comments:
I'm not sure about the rename of initbuild.sh cmake vs initbuild.sh ninja because users might also uses cmake with make. AFAIR initbuild.sh make still uses cmake to create make files, so initbuild.sh ninja would be consistent. More importantly, if there is no clear preference, as you can argue either way, it is probably better to not make change to avoid affecting other users.
I will agree that initbuild.sh meson is a bit different, but this could be initbuild.sh meson, initbuild.sh meson-ninja, initbuild.sh meson-make if we want to add that. But I think initbuild.sh meson is fine. You can then change backend with meson as you prefer.

I just got meson 1.2.2 on MacOS Sonoma. I get some warnings. I have not tried to examine these closely, so this is just FYI for now, and non-exhaustive:
scripts/initbuild.sh meson
...
../../meson.build:108: WARNING: Consider using the built-in warning_level option instead of using "-Wextra".
../../meson.build:108: WARNING: Consider using the built-in werror option instead of using "-Werror".
Found ninja-1.11.1 at /opt/homebrew/bin/ninja
WARNING: Running the setup command as meson [options] instead of meson setup [options] is ambiguous and deprecated.

When subsequently running
scripts/build.sh
...
ld: warning: -undefined error is deprecated

I'm not sure about these ld warnings, could also be my machine, but I think / thought I just got that cleaned up.

That said
scripts/build.sh and scripts/test.sh both run as expected.

I'm getting some 8+ second execution time on meson with scripts/test.sh (which also cleans and rebuilds on both debug and release), and 9+ secons with cmake, so in same ballpark, both using meson.

I like that you have set c_std=c11 and cpp_std=c++11 in root meson.build. This is the target platform.
NOTE: there might be errors showing up for users choosing other build options, but we cannot test all options. I do note that you have copied gcc version checking in meson build, so at least it is likely to work in most cases. At any rate, for the project to be fully portably it cannot rely purely no c11 - the portable library has many checks for which compiler system is being used and some workarounds wrt. warnings, which might be be fully reflected in the meson build. I have not looked into this, this is just a heads up. We might have to make a note of that in the README, but as always, new build variants depend on user contribution and testing.

Wrt. meson and the build rules from the old meson branch 0.4, I trust you have testet dependencies including trying to change an indirect header or included schema file, otherwise please verify this. I'm not going into this level of detail in my review.

[mikkelfj]

I don't know why the build doesn't run, as I wrote above, but I made a temporary separate branch and merged with latest and master and it builds fine.

EDIT: I spoke too soon. I ran the weekly build and all standard builds passes, but the build to check CMake version fails.
https://github.com/dvidelabs/flatcc/actions/runs/6615150857/job/17966976366

The same build does pass on the main branch:
https://github.com/dvidelabs/flatcc/actions/runs/6611885065/job/17956649724

I'm note sure if it is CMake version related, but we will probably upgrade min CMake.

[mikkelfj]

I made a new commit on master d533710 to handle the case in your commit bcc1c3e , but I did it by ensuring there never is a null pointer but rather an empty buffer.

[mikkelfj]

I did not look through all benchmark code, but some casts you have made are semantically different from the original:
I'm not sure it makes a difference in practical terms, but since you went through the effort of adding casts, you might as well do it correctly.

 FooBar(sibling_create(B,
                0xABADCAFEABADCAFE + i, 10000 + i, '@' + i, 1000000 + i,
                123456 + i, 3.14159f + i, 10000 + i));

vs

FooBar(sibling_create(B,
               0xABADCAFEABADCAFE + (unsigned long)i,
               10000 + (short)i,
               '@' + (char)i,
               1000000 + (unsigned int)i,
               123456 + i,
               3.14159f + (float)i,
               10000 + (unsigned short)i));

There are other examples like FooBar(postfix_add(B, '!' + (unsigned char)i)) but here you use unsigned.
Overflowing truncated integers is not necessarily the same as truncating before summing. The char type itself may or may not be signed AFAIR. NOTE that overflowing a signed type by adding two values where the result is e.g. too large for the type is UB while truncating a larger type to a smaller after summing via cast is not UB.

[amcn]

Thank you for your feedback! I will take another look at this PR and integrate your feedback as soon as possible. It has been a few months since I looked at this PR, so I will need some time to refresh my memory.

Just as a side-note: I implemented this branch because I needed something like flatcc for a work project which is built via meson. After this PR was pushed I started making use of this branch in this work project and I would like to thank you for your work on flatcc: it's been a very nice library to work with.

I absolutely do not want to break anything for existing users, so I will make especially sure to take your feedback about that into account.

[mikkelfj]

@amcn I'm glad it worked out for you, and thanks for working on this.
I think I will make a release before updating the build system.
I also have this #PR258 for improving CMake incremental builds which has stalled on the CMake version, and upgrading CMake also breaking Appveyor build.
I will likely add that and you PR after the next release, which I hope to do soon.
Note that we should also have some meson build on the weekly.yml GH action file.

Wrt. sanitizers, there have been other PRs like #237, and commits on this topic. So I just made the commit 0e925e1 to add the sanitizer flag to clang debug and fix source code accordingly (which still does not address benchmarks).

[WillAyd]

@amcn are you still working on this PR? We use flatcc downstream in our nanoarrow library and have also been using the Meson wrapdb for distribution, so this would be a great fit for us.

Happy to try and pick up where this left off and push over the finish line if you could use help

[amcn]

@mikkelfj Apologies for not having come back to this, I had forgotten admittedly. My original intention was motivated by a need I had at my previous workplace and in the intervening time that company and its product have since ceased to exist.

Incidentally @WillAyd, I was using flatcc so that I could implement an Arrow IPC interface in C, much like nanoarrow (although nanoarrow's implementation seems to be much better than mine). It's a funny old world.

I will take another look at this and try to address as many of the comments as possible in order to get it ready for merging.

[WillAyd]

I don't think you need to add default_library=static here; the user can configure how they want to do this via a command line option, or Meson will intelligently handle this for you away (i.e. if used as a subproject, Meson will build this as static anyway)

[amcn]

Found some time to work on this. So the reason I added default_library=static originally is to make Windows build correctly on the CI. Ultimately without this, Meson's library() generates a DLL. Given that flatcc currently does not explicitly export any symbols from this DLL by setting __declspec(dllexport) on any symbols, MSVC does not create any import lib flatcc.lib causing subsequent links to this library to fail, notably the flatcc cli tool. By forcing the generation of a static library, the link problems are avoided.

I've been trying various ways to get around this including both_libraries(), but nothing seems to produce a linkable library on Windows. I think that the best path forward is to leave this as it is default_library=static specified in the toplevel Meson script, and if users want to specify --default-library=shared on the command-line then they are free to, but with the knowledge that this will cause Windows link failures. This mirrors CMake's -DBUILD_SHARED_LIBS=on option, which when activated with flatcc also causes Windows link failures due to missing import libs.

I have a local patch with explicitly adds __declspec(dllexport) to flatcc's public API, which seems to resolve the issue for both Meson and CMake, but if such a patch is ever considered for submission to flatcc, I think it would be best to do it separately from this PR. It touches lots of files.

TL;DR: On Windows, neither Meson nor CMake is currently capable of building and linking a shared version of flatcc, so default to static under Meson as is already the case under CMake.

[WillAyd]

Ah OK that makes sense. I ran into a similar issue with nanoarrow last month - the advice I received from upstream was to do something like:

if is_windows
    libtype = 'static_library'
else
    libtype = 'library'
endif

build_target(
    ....,
    target_type: libtype,
)

mesonbuild/wrapdb#1536 (comment)

[WillAyd]

Of course could also try to properly export symbols and simulate the issues by setting -fvisibility=hidden for gcc, but that would require changes to the flatcc code itself

[WillAyd]

Although common in CMake you don't need to prefix the options with flatcc; if needed in Meson you can add the library name as a prefix to things anyway, so -Dflatcc:portable would look for the flatcc library and set the portable flag.

Of course if you are just compiling from flatcc you don't need to specify this, and could just go with the shorter -Dportable= command line argument

[amcn]

Makes sense, I've removed the flatcc_ prefix for all the options as you suggested. I'll push up a new version of this PR in the near future once I have implemented as much of the feedback as possible.

[mikkelfj]

@amcn Sorry I have not been able to respond timely. I will get back to you on this when I am able focus more on this project, if you are still up for it.

[amcn]

Apologies, the inherent trauma of the human experience has been a particularly heavy burden these last few months and I have neglected many responsibilities as a result.

I do still aim to get this into shape so that it can be merged and others can make use of it.

[amcn]

I noticed something when working on getting the Windows side of this PR into shape: there seems to be unreachable code in src/compiler/codegen_schema.c.

msvc looks at this block between the two breaks and raises a warning. From a glance at the code, its analysis seems valid. I know this isn't strictly related to this PR, but I thought I would bring it to your attention as the code definitely looks strange to me.

[mikkelfj]

Thanks for looking into this. Yes it does look suspicious. It it is probably the least bad place it could be though, since it seems to only affect the binary schema generated for a not so often used schema feature. Probably needs a review to see if the skipped code is actually correct before removing the break. Could be a bad merge.

I too sense the flux of the essence of being, but will try to get back to you on this PR soon.