Add Github CI

[bjosv]

Add Github CI and weekly builds to run tests with a range of different compilers and compiler versions on Ubuntu, Mac and Windows.

This PR includes:

• Updated minimum required CMake version from 2.8 to 2.8.12.2
• Fixes to build with GCC on Mac.
• Fixes to build with Clang 3.9, 4.0, 5.0
• Additional fixes in test for GCC 32-bit

[bjosv]

Is this inline with what we need in the project? Currently the build would fail due to some compiler versions don't compile or the test fails, but I will look into that. Some build logs: Ubuntu: https://github.com/Nordix/flatcc/actions/runs/3315747970 Mac: https://github.com/Nordix/flatcc/actions/runs/3315747963

[mikkelfj]

Haven't reviewed yet, but this sounds awesome. GCC on Mac is somewhat pointless since it is just Clang pretending. But since you ran into issues, maybe it is not pointless after all. Strange with Clang errors? But could be regressions due to CI downtime.

[mikkelfj]

MacOS GCC, note line 339 in log: 1/22 Test #1: monster_test_cpp .................Subprocess aborted***Exception:. All subsequent errors with buffer too small is likely because something goes wrong when writing the buffer.

[mikkelfj]

We also the need the banner for the build in README.

I am wondering if we should limit builds to certain branches - I think it is probably not constructive in this context although I strongly prefer it in production because it might otherwise trigger unwanted deployments etc.. However, the old Travis CI setup did have two build types: one with just the latest few compilers and one that did the whole thing, but it took a long time and I only used it before making a release to check against regressions. It also depends on GH speed.

I looked at the Ubuntu GH script. I strongly prefer Ninja for development, but I think Make is better for cross platform validation. I think the Travis build used Make. Ideally at least a few configurations would do both, but that might be overkill.

So this is more thoughts than requirements.

[mikkelfj]

It is the ci-more branch where I add more test on Linux and macOS. There is something similar for Windows on Appveyor, but I think it stranded a bit because Appveyor changed their environment so I just chose the most important middle of the road builds there.

[mikkelfj]

Also, if you haven't already. C++ tests are important. There is only a single test that can be disabled, but I got a lot of bug reports from C++ users until someone contributed that test. This avoids small regressions that breaks C++ that I wouldn't normally check for. Note that C++ test just means a regular C test included in C++ file and compiled with g++ or clang in C++ mode. I'm not sure C++ pass all compiler versions, but at least the recent ones and possible some older that currently works.

[mikkelfj]

As to ci-more - if we keep that concept, I definitely would want only one version the GH script with branch detection within the script rather than merge controlled differences as it is today - that is very fragile.

[bjosv]

I have been away, but I will try to get some progress on this and look into the possible issues.

Building with make as well makes sense, good idea. I'll check if all C++ tests are running as well..

Currently the CI run is quite quick, but since it might grow if we add sanitizers or valgrind runs it might be a good idea to have a smaller workflow for PRs/commits and a thorough workflow for releases. Github supports that a workflow can be started manually only, so maybe that could be a good approach for the larger "release" workflow. Both workflow exists in the master branch, but only the smaller one is automatically run. This would avoid having a ci-more branch unless there are other changes needed.

[mikkelfj]

Sounds good. I think it makes sense to make the default automatic build a little larger than minimal if it is fast enough, as long as it adds value. We are not hunting for regressions, but e.g. latest and typical gcc tend to differ sometimes.

[mikkelfj]

Maybe the large build can be scheduled weekly or something?

[bjosv]

Sure weekly works as well, this would be good if external analysis tools like https://scan.coverity.com/ is added later. Coverity is pretty nice and gives pretty good warnings, but it has the caveat to only allow few runs per day/week for free. I have a weekly run of it using Github Actions in another project. Might be something for the future.

[bjosv]

Some updates to have a CI run of basic targets and a weekly run with a wider range.

• Checked that all except gcc-4.4 runs the C++ test.
• Added usage of make for some jobs but maybe I should add some more in the weekly runs.
• Added a job for intels icc compiler handled in the CMakeLists
• Fixed the issues found regarding gcc on mac, and in older Clang on Linux

Maybe there could be some more runs of windows in weekly..

Examples: https://github.com/Nordix/flatcc/actions/runs/3469147023 https://github.com/Nordix/flatcc/actions/runs/3469147024

[mikkelfj]

This looks great. If we are going to switch from AppVeyor, we definitely need more 32 and 64 bit Windows builds weekly, down to at least MSVC 2013. Currently we go back to MSVC 2010, but I'm ready to drop that.

As long as you keep the commits clean I think it is fine as is, but otherwise source code fixes should preferably be in a separate PR.

Wish list: I'm not sure if we did have 32 bit Clang or GCC builds, but I think a 32-bit build is in place to ensure integer types behave as they should. Eventually ARM builds would also be more relevant.

[mikkelfj]

BTW: I assume you noticed the annotations in yourr weekly build example: https://github.com/Nordix/flatcc/actions/runs/3469147024

[bjosv]

32-bit is a good idea, I'm not sure the best way to get the -m32 flag in there though, maybe one should give cmake a toolchain file or something.

I looked a bit at ARM but most are using own runners, but a cross-compilation seems to work which can be added.

I'll see if I can fix some of the annotation as well, the used install-clang-helper gives most warnings.

[mikkelfj]

ARM is probably most interesting on an ARM OS where tests can also run. But plenty of users have used ARM based systems with not issues so far, or at least the issues have been elsewhere with stdlib version etc.

[mikkelfj]

I looked at the build logs from ICC in your weekly example. There are warnings that do not surface in the build overview (annotations), for example:

/home/runner/work/flatcc/flatcc/src/runtime/builder.c(1918): warning #188: enumerated type mixed with another type
      return B->frame ? frame(type) : flatcc_builder_empty;
             ^

/home/runner/work/flatcc/flatcc/src/runtime/builder.c(1926): warning #188: enumerated type mixed with another type
      return B->frame[level - B->level].type;
             ^

Also, it may be difficult to continue building with ICC, given the following from same build:

cc: remark #10441: The Intel(R) C++ Compiler Classic (ICC) is deprecated and will be removed from product release in the second half of 2023. The Intel(R) oneAPI DPC++/C++ Compiler (ICX) is the recommended compiler moving forward. Please transition to use this compiler. Use '-diag-disable=10441' to disable this message.

[mikkelfj]

Can you incorporate #217 (update CMake version) in this PR? Could easily add after, but it would be good to know that it works on all builds.

[bjosv]

Sure, I can add the CMake version update commit. In the coming change I have added a job (weekly) that builds using cmake 2.8 to make sure the minimum required version works on ubuntu.

The annotations issue are gone by replacing the used github-helper and fixating some run-on versions. For icc there are older version of the compiler available, so I changed to an older one and more warnings seen, but no notice of deprecation. So I guess the job can run until the compiler is decided to be deleted from the public. We are not currently using -Werror in that branch in CMakeLists, so warnings will not be shown in the github annotations, just in the logs.

Just fiddling with the 32bit builds now..

[bjosv]

Current example runs: CI: https://github.com/Nordix/flatcc/actions/runs/3491255367 Weekly https://github.com/Nordix/flatcc/actions/runs/3491255369

[mikkelfj]

I assume that you are still looking at 32-bit build, but otherwise - there is no test running for gcc 32-bit, only a check for the ./bin/flatcc file if I read it correctly. I also made on comment on a missing description for the weekly build status in README.

(EDIT: ah - now Github decided to show my comment on the README ...)

[mikkelfj]

The Intel build runs both ICC ICX in the same build. I wonder if there is any good reason for that and how that affects reporting.

[mikkelfj]

I also wonder if we should entirely skip the build for 2.8, or upgrade it to 2.8.12?

[mikkelfj]

I note that ICX has zero warnings in build log, and ICC has an almost endless amount. Since Intel is completely switching over to ICX (clang based) I guess we can safely ignore those warnings.

[bjosv]

I assume that you are still looking at 32-bit build, but otherwise - there is no test running for gcc 32-bit

I got problems with 2 tests in the test runs (log) with I'll look at.

The Intel build runs both ICC ICX in the same build.

I put them in the same job to avoid downloading the compiler and duplication of the prepare step, but it can be split in two if you think that is better. One drawback with current single job this is that if icc fails then there is no attempt to build with icx. If the job is duplicated then they run in parallel indifferent of each other.

I also wonder if we should entirely skip the build for 2.8, or upgrade it to 2.8.12?

CMake 2.8.12.2 is used in both cases, but when I set 2.8.12.2 for the actions-setup-cmake it fails to find it weirdly enough. Using 2.8.12 works when tested, so I'll change it to that. Will file an issue regarding this problem to the actions-setup-cmake project.

[mikkelfj]

My main concern with ICC if it is unsupported in the future and we want to stick with ICX, it is unclear what is necessary in the earlier build step in terms of downloads. And then of course if one fails the other is blind. So I think it is best to split since it is only weekly. It might also be possible to trigger a single sub build when tracking problems, not sure.

[bjosv]

Building with 32bit with clang seems fine, so there is something with gcc that triggers problems with a float compare in an assert (emit_test) and something else similar in monster_test.

[mikkelfj]

I'm looking at it right now, none the wiser: https://github.com/dvidelabs/flatcc/blob/821cfa648e4d8ba50d411b7f1620ed7bea60d616/test/emit_test/emit_test.c#L115

It suggests we should probably have both a clang and a gcc 32 bit test? Mind you, MSVC has been running 32-bit tests successfully for a very long time on Appveyor.

[bjosv]

Adding -ffast-math to GCC seems to make it work due to changed precision, described a bit in link. But I'm not an expert in this area..

[mikkelfj]

The other test that fails is: https://github.com/dvidelabs/flatcc/blob/821cfa648e4d8ba50d411b7f1620ed7bea60d616/test/monster_test/monster_test.c#L374-L378

[bjosv]

Yes, in monster_test when printing out the values with %a in GCC it becomes: if (-0x1.99999ap+1 != -0x1.999999999999ap+1) { but from clang if (-0x1.99999ap+1 != -0x1.99999ap+1) {

[mikkelfj]

point 4. in the this post: https://stackoverflow.com/a/10335601

Seems like potentially a double rounding issue from high to low precision?

[mikkelfj]

if (-0x1.99999ap+1 != -0x1.999999999999ap+1) {

When you write 3.2f and not 3.2d, you mean mean 3.2f, how hard can it be?

[mikkelfj]

Anyway, printf is not a good test - not sure about %a, but as I recall it only takes double as argument, so value has to be cast to double implicitly before printing.

[mikkelfj]

I think we need to fix the tests somehow, just not sure how. -ffast-math seams a bit like a moving target.

[bjosv]

Yes, printf if probably not the best test, as you say, there is probably some casting done..

[mikkelfj]

We could add parse_float_is_equal and parse_double_is_equal to https://github.com/dvidelabs/flatcc/blob/master/include/flatcc/portable/pparsefp.h

[mikkelfj]

Proposed addition to https://github.com/dvidelabs/flatcc/blob/master/include/flatcc/portable/pparsefp.h

/* Work around GCC double precision conversion of float values. */
#ifdef GCC_... tbd

static inline int parse_float_is_equal(float x1, float x2)
{
    union { uint32_t u32; float f32; } v1;
    union { uint32_t u32; float f32; } v2;
    v1.f32 = x1;
    v2.f32 = x2;
    return v1.u32 == v2.u32;
}

#else

static inline int parse_float_is_equal(float x1, float x2)
{
    return x1 == x2;
}

#endif

static inline int parse_double_is_equal(double x1, double x2)
{
    return x1 == x2;
}

[bjosv]

Nice, I'll try adding that.

[mikkelfj]

Note I changed gcc to GCC in comment.

[mikkelfj]

If that doesn't work we might have to add some EPSILON to each input value before casting to u32 to force lower bits to flip. Possiby casting to double before doing that.

[mikkelfj]

Some more background here: https://bitbashing.io/comparing-floats.html But I or we can improve if we get the tests running and plugging in the compare function.

[mikkelfj]

Also a some discussion here: https://stackoverflow.com/questions/5989191/compare-two-floats

[mikkelfj]

I added parse_float_compare and parse_float_is_equal to pparsefp.h file on master. Currently it is always taking slow path on 32-bit, i.e. no GCC specialization.

[mikkelfj]

I pushed fixes to emit_test and monster_test on master. Also improved on the initial pparsefp.h impl. Comparison now drops least significant bit in tests via parse_float_is_equal.

[bjosv]

Ah great! Had to look at something else for some time, will rebase it in.

[mikkelfj]

Just quick feedback on stuff noticed: 32 bit build steps are named something like "run build", while the others are "run build and test". 32-bit builds (and windows) does apparently not test both debug and release while scripts/test.sh used in most build variants should run both.

[bjosv]

Updates in weekly:

• split Intel icc and icx jobs (via a matrix)
• CMake version specified as 2.8.12 (results in 2.8.12.2 which cant be set explicitly, some bug)
• 32bit builds on both GCC and Clang (current default version)

Added a fix for some more changes needed in monster_test. Added description to readme.

Logs from latest run: Weekly: https://github.com/Nordix/flatcc/actions/runs/3497788685 CI: https://github.com/Nordix/flatcc/actions/runs/3497788688

[mikkelfj]

And maybe the min required build should be pushed back to runs on Ubuntu 20.04. I think 18.04 might fail. Then again, who knows how long Github supports that build ...

[mikkelfj]

Oh, we are very close to have this going now ... Thanks for all the hard work so far.