Add Github CI by bjosv · Pull Request #250 · dvidelabs/flatcc

bjosv · 2022-10-24T20:22:35Z

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 · 2022-10-24T20:26:59Z

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 · 2022-10-24T20:27:20Z

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 · 2022-10-24T20:30:31Z

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 · 2022-10-24T20:42:21Z

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 · 2022-10-24T20:45:54Z

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 · 2022-10-24T20:55:03Z

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 · 2022-10-24T21:05:25Z

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 · 2022-11-08T17:07:11Z

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 · 2022-11-08T17:12:27Z

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 · 2022-11-08T17:13:42Z

Maybe the large build can be scheduled weekly or something?

bjosv · 2022-11-08T17:26:27Z

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 · 2022-11-15T10:03:14Z

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 · 2022-11-15T10:19:01Z

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 · 2022-11-15T10:28:47Z

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

bjosv · 2022-11-15T10:46:26Z

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 · 2022-11-15T10:48:53Z

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 · 2022-11-15T17:04:29Z

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 · 2022-11-17T17:29:10Z

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 · 2022-11-17T18:05:35Z

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 · 2022-11-17T19:35:16Z

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

mikkelfj · 2022-11-17T19:42:22Z

README.md

-OS-X & Ubuntu: [![Build Status](https://travis-ci.org/dvidelabs/flatcc.svg?branch=master)](https://travis-ci.org/dvidelabs/flatcc)
+Ubuntu, macOS and Windows: [![Build Status](https://github.com/dvidelabs/flatcc/actions/workflows/ci.yml/badge.svg)](https://github.com/dvidelabs/flatcc/actions/workflows/ci.yml)
 Windows: [![Windows Build Status](https://ci.appveyor.com/api/projects/status/github/dvidelabs/flatcc?branch=master&svg=true)](https://ci.appveyor.com/project/dvidelabs/flatcc)
+[![Build Status](https://github.com/dvidelabs/flatcc/actions/workflows/weekly.yml/badge.svg)](https://github.com/dvidelabs/flatcc/actions/workflows/weekly.yml)


Missing a description for the weekly build I think.

Here is an example how the image looks:

I skipped a description since the text Weekly is included in the image, but maybe there is a need for a description to get some distance to the windows badge.

Well, I cannot see how it looks yet so I am just raising a potential concern, but I think it makes sense that all three builds have some text since otherwise it might see as if it is related to the Windows build, I guess. You could call extended build or something.

mikkelfj · 2022-11-17T19:46:44Z

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 · 2022-11-18T16:59:36Z

But I won't stop you if you have something better for Windows.

bjosv · 2022-11-18T17:01:49Z

Ok, seemed that it was not possible to get any version anyway, but
For windows-latest or windows-2022 that will be Visual Studio 2022. And for windows-2019 that will be Visual Studio 2019

Can copy the job we have, and run on both images to get two VS versions at least.

mikkelfj · 2022-11-18T17:02:15Z

Also, you might want to update your initial PR comment since it isn't entirely up to date, yet rather specific.

mikkelfj · 2022-11-18T17:08:40Z

I have tested running builds on much older MSVC versions on GH actions, but it is a bit involved and some time ago, so I don't want to go into that for now. Also, it was not with CMake. But I have some material I can dig into.

bjosv · 2022-11-18T17:13:12Z

Perfect!
I'll try to tying up the loose ends and finalize this later, or during the weekend, it's time for a beer now :)

mikkelfj · 2022-11-18T17:13:37Z

Great, and thanks again for the huge effort.

bjosv · 2022-11-21T09:47:08Z

Unfortunately the weekend flew by.., but here is the changes:

Added Windows to weekly
Added Mac Clang to weekly
Some notes in README about the new build configuration available via initbuild.sh
Simplified the inital PR comment

Example of
Weekly: https://github.com/Nordix/flatcc/actions/runs/3513124399
CI: https://github.com/Nordix/flatcc/actions/runs/3513124396

mikkelfj · 2022-11-21T10:19:46Z

This looks good. Some quick observations:

Windows has warnings in pparsefp, likely due to changes I made - these warnings are also hard to find in GH actions - appveyor is much better at destilling these.

ci build seems to run Ubuntu 20, while this is relevant for some old test, we probably want to bump to 22 or so, if possible, for latest compiler tests?

icc compiler warnings appear to be concentrated on a single const expression warning, so perhaps we can find and disable that warning?

GCC 4.4 has the following warning:
/home/runner/work/flatcc/flatcc/src/compiler/semantics.c:1549: warning: missing initializer
/home/runner/work/flatcc/flatcc/src/compiler/semantics.c:1549: warning: (near initialization for ‘index..s.len’)
Maybe not too important, but worth investigating.

Overall, I think this is good enough to merge now, what do you think?

mikkelfj · 2022-11-21T10:21:57Z

You can see the Appveyor warnings here:
https://ci.appveyor.com/project/dvidelabs/flatcc/build/job/d6bt1ss79irvrt86/messages

Do you think GH Actions has some similar filter?

mikkelfj · 2022-11-21T10:28:57Z

Also, I think weekly.yml is currently set to build on push and pull requests.

bjosv · 2022-11-21T10:45:00Z

Ah, the warnings-tab in appveyor looks usable. I haven't seen any similar on GH Actions yet though..
I'll update to ubuntu22 in CI before we merge.

The on: in the files describes when to run the workflow, and in CI its on push and pull_requests.
In weekly its on workflow_dispatch which enables manual runs of the workflow from the gui, and then the schedule.
I had to add push and pull_request temporary when testing, but to me it looks like I managed to remove them before pushing to the PR branch. The examples are taken from the branch with the temporary change.

Also introduces a new build config for forced 32bit builds: > scripts/initbuild.sh make-32bit

aligned_alloc() seems not fully supported when using GCC on Mac, so use posix_memalign() instead. aligned_alloc() returns null in tests and https://en.cppreference.com/w/c/memory/aligned_alloc states: ".. not supported by the implementation causes the function to fail and return a null pointer"

semantics.c:1551:32: error: missing field 'len' initializer fb_value_t index = { { { 0 } }, 0, 0 }; See https://reviews.llvm.org/D28148 and related https://reviews.llvm.org/rL314838

So update to 2.8.12.2 (in Ubuntu 14.04) CentOS 7 has a way to install CMake3. This avoids the warning in newer CMakes: flatcc/CMakeLists.txt:5 (cmake_minimum_required): Compatibility with CMake < 2.8.12 will be removed from a future version of CMake.

mikkelfj · 2022-11-21T10:50:52Z

I'm just thinking: I don't want weekly to run on PR because it takes too long for feedback, and trigger a lot of builds with frequent pushes, on other hand, it currently only takes 6 minutes to build, and I probably cannot trigger builds in PR branches since it is not my repo, or?

Most of the time I would assume things are good if ci.yml is good, but there might be cases where portability is more of a concern.

bjosv · 2022-11-21T11:09:44Z

You are probably right, I've never attempted to run a workflow manually on a PR before, so it might be problematic. Not sure..cant find any examples describing it either.
This would for sure be nice to do when a PR is believed to affect a lot..

mikkelfj · 2022-11-21T11:28:49Z

Any idea how to add the ci.yml build to PR checks? I had this for Travis and Appveyor, and recently just Appveyor, but I am a bit at a loss how to enable this with GH actions?

mikkelfj · 2022-11-21T11:30:00Z

Or is it already enabled for future PRs? I don't see it on existing PRs.

bjosv · 2022-11-21T12:23:08Z

I believe it's only triggered by events, like new PRs or changes/push in existing PRs..

bjosv · 2022-11-21T12:29:32Z

So an update by committer or admin like this should make it run.

mikkelfj · 2022-11-21T12:39:18Z

OK, thanks, we'll see how it goes.

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

bjosv force-pushed the github-ci branch from d58e6bb to 1356642 Compare November 15, 2022 09:54

bjosv force-pushed the github-ci branch from 1356642 to 0235df6 Compare November 17, 2022 19:22

bjosv marked this pull request as ready for review November 17, 2022 19:23

bjosv changed the title ~~WIP: Add Github CI~~ Add Github CI Nov 17, 2022

mikkelfj reviewed Nov 17, 2022

View reviewed changes

bjosv force-pushed the github-ci branch from f683394 to f2a0a2a Compare November 21, 2022 09:38

bjosv and others added 6 commits November 21, 2022 11:46

Add Github CI and weekly test runs

a6ba394

Also introduces a new build config for forced 32bit builds: > scripts/initbuild.sh make-32bit

Include clang 3-4 in precision warning workaround

30712d9

Suppress missing-field-initializers warning pre-clang6

16ac66a

semantics.c:1551:32: error: missing field 'len' initializer fb_value_t index = { { { 0 } }, 0, 0 }; See https://reviews.llvm.org/D28148 and related https://reviews.llvm.org/rL314838

Add more lowered precision tests of floats (for GCC 32-bit)

53886d2

bjosv force-pushed the github-ci branch from f2a0a2a to 53886d2 Compare November 21, 2022 10:47

mikkelfj merged commit 1054d66 into dvidelabs:master Nov 21, 2022

bjosv deleted the github-ci branch November 21, 2022 11:12

mikkelfj mentioned this pull request Nov 21, 2022

Newer CMake is going to drop <2.8.12 #217

Closed

mikkelfj mentioned this pull request Dec 7, 2022

cmake: flatcc_generate_sources + add_custom_command + crossbuild + ci #171

Closed

Conversation

bjosv commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjosv commented Oct 24, 2022

Uh oh!

mikkelfj commented Oct 24, 2022

Uh oh!

mikkelfj commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikkelfj commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikkelfj commented Oct 24, 2022

Uh oh!

mikkelfj commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikkelfj commented Oct 24, 2022

Uh oh!

bjosv commented Nov 8, 2022

Uh oh!

mikkelfj commented Nov 8, 2022

Uh oh!

mikkelfj commented Nov 8, 2022

Uh oh!

bjosv commented Nov 8, 2022

Uh oh!

bjosv commented Nov 15, 2022

Uh oh!

mikkelfj commented Nov 15, 2022

Uh oh!

mikkelfj commented Nov 15, 2022

Uh oh!

bjosv commented Nov 15, 2022

Uh oh!

mikkelfj commented Nov 15, 2022

Uh oh!

mikkelfj commented Nov 15, 2022

Uh oh!

mikkelfj commented Nov 17, 2022

Uh oh!

bjosv commented Nov 17, 2022

Uh oh!

bjosv commented Nov 17, 2022

Uh oh!

mikkelfj Nov 17, 2022

Choose a reason for hiding this comment

Uh oh!

bjosv Nov 18, 2022

Choose a reason for hiding this comment

Uh oh!

mikkelfj Nov 18, 2022

Choose a reason for hiding this comment

Uh oh!

mikkelfj commented Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikkelfj commented Nov 18, 2022

Uh oh!

bjosv commented Nov 18, 2022

Uh oh!

mikkelfj commented Nov 18, 2022

Uh oh!

mikkelfj commented Nov 18, 2022

Uh oh!

bjosv commented Nov 18, 2022

Uh oh!

mikkelfj commented Nov 18, 2022

Uh oh!

bjosv commented Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikkelfj commented Nov 21, 2022

Uh oh!

mikkelfj commented Nov 21, 2022

Uh oh!

mikkelfj commented Nov 21, 2022

Uh oh!

bjosv commented Oct 24, 2022 •

edited

Loading

mikkelfj commented Oct 24, 2022 •

edited

Loading

mikkelfj commented Oct 24, 2022 •

edited

Loading

mikkelfj commented Oct 24, 2022 •

edited

Loading

mikkelfj commented Nov 17, 2022 •

edited

Loading

bjosv commented Nov 21, 2022 •

edited

Loading

bjosv commented Nov 21, 2022 •

edited

Loading