Ha, just today I got something working that I haven't put in a pr request yet for. I haven't yet had a chance to look at yours but it sounds like we've duplicated some bits, but we also both have stuff the other doesn't.

I've been doing the work in a pull request against my own fork here dbaarda#1.

I've got windows builds and tests including rdiff working with vcpkg popt! And man, that was an adventure!

I'll put together a pull request of my stuff and go through yours tomorrow, and see how we can merge our efforts.

I saw that you've assigned this ticket to you, but have not found the branch in progress. 😄

Anyway, feel free to use this PR as is, or split it up. And I am happy to work on it if you need me.

• I've fixed the CMake warning in a separate PR (cmake: fix warning for later CMake versions #221)
• Tried to reproduce the travis build as closely as possible
• I've added 3 extra workflows, which are using Clang tools
  • Clang-tidy is produces the reports to console. (does not break the build)
  • Clang's static analyzer produces detailed output into a report directory. The console output is a short version of the detailed output, can be used as a signal to run the command locally. (does not break the build)
  • "Include what you use" produce a verbose output to the console. (does not break the build)

I was working in a PR on my own fork because I used github's UI for adding a new workflow and it asked if I wanted to make a PR out of it, I answered "yes", and to my surprise it created the PR against my own fork, not the main upstream librsync. Since I had no idea what I was doing, I figured I might as well keep fiddling with it there since there was a high chance it would turn into a disaster that needed to be abandoned anyway. In the end I did get it working, but if you look at the commit history it's full of failed experiments.

I was thinking I would just make a new PR out of the branch, but seeing your work I'm thinking now of splitting it up into digestible pieces. I also have some questions about how you did things that I'd like to bounce off you because I'm new to this.

1. I see you reproduced the old travis build as closely as possible. I started with the recommended CI workflow which included doing the build in a subdirectory instead of in-place, and explicitly specifying a build-type. I believe this is currently best-practice, so I thought I'd stick with what they recommend. The only problem I found is the docs build then fails to find the markdown files in the repository root, and I haven't yet figured out how to fix this. Do you have any strong opinions on what is better here? In-place builds are a bit simpler and we inherited them from the old autoconf days, but perhaps it's time to update? Explicitly specifying the build-type is probably also better, and I really want to add tests for Debug builds since it enables all the assertions that could catch problems Release builds miss.
2. I see neither of us have added any caching of installed packages. Is this worth adding? I found everything much faster than travis, but I was also hammering it with CL's testing stuff so faster would have been nicer. I guess most of the time we would not be doing this, so is the added complexity worth it? It would be nice if there was a standard action for this, but my search showed they are still in a bit of a fragmented mess.
3. Do we care about testing Ninja? Is it really likely we could inadvertently break this? How much value does this test add?
4. You are setting a CMAKE_OPTIONS environment variable and using it in your invocation of cmake. Is there a reason this is better than just passing the arguments to cmake?
5. You are targeting particular versions of platforms, but I just went with "*_latest" as per the recommended workflow. Is there a good reason to pick particular versions? In the past I've had to update travis because versions ceased to be supported, so wouldn't latest be better?
6. You are always specifying the compiler to use, whereas I use the platform default compiler unless I explicitly want to test another compiler. In general I really do want to test using the default compiler as the best representative case, but I also want to test using different (clang) compilers. Particularly on non-linux platforms, I'm not even sure what the default compiler is. There is a risk that the default compiler is the same as an explicitly selected compiler so I just test the same compiler twice. Always explicitly specifying the compiler makes the workflow simpler. I tested what happens if compiler="" in the matrix and -D CMAKE_C_COMPILER="" gives a cmake error. In mine I don't have a compiler entry in the matrix and just put the -D CMAKE_C_COMPILER=clang in the options. Thought on what's best here?
7. When installing packages you do apt-get update -y to update the package lists, and I don't bother, avoiding the update overheads and just using whatever the os image is updated to. How important is doing the update? In some ways just using the image gives us more repeatable results, but getting the updates ensures we are more up to date. Thoughts?
8. You are installing cmake, but I'm using the cmake installed on the image. Is this just to ensure you have an up-to-date cmake? Can we just rely on the os image?

Hey @dbaarda , good to see that many questions. 😄

7. When installing packages you do apt-get update -y to update the package lists, and I don't bother, avoiding the update overheads and just using whatever the os image is updated to. How important is doing the update? In some ways just using the image gives us more repeatable results, but getting the updates ensures we are more up to date. Thoughts?

I have burned my fingers before to not update the base image. The problem could be that you want to install a package (the latest version of the package) and it won't work with the installed dependencies. This problem does not happen too frequently, but there are chances. I ran into this when I wanted to release something quick, and then had to spend extra hours to find out what's wrong.

8. You are installing cmake, but I'm using the cmake installed on the image. Is this just to ensure you have an up-to-date cmake? Can we just rely on the os image?

Both is fine. I like explicitly name the build dependencies... I found it also useful, to reference the CI script to users, who want to build the project.

My thoughts on the apt-get update -y and the apt-get install cmake, that they are cheap for this project. (~20 seconds for ubuntu images)

2. I see neither of us have added any caching of installed packages. Is this worth adding? I found everything much faster than travis, but I was also hammering it with CL's testing stuff so faster would have been nicer. I guess most of the time we would not be doing this, so is the added complexity worth it? It would be nice if there was a standard action for this, but my search showed they are still in a bit of a fragmented mess.

I did not do any caching on GitHub. I think for this project, the builds are reasonably fast. (I have more heavy weight projects.)

If you want to use caching not for speed, but to create a stable environment when only the code changes, I would recommend to use docker images. Where the image contains all dependencies (build and runtime). Not sure if this is also the fastest, because you need to load the docker image, but no need to install packages.

I found the purpose of the CI job is to catch the build environment changes too. Those are the errors which I won't catch on my development machine. Stable build environments (like dockerized builds) are useful for big project, with many dependencies and long compilation times.

1. I see you reproduced the old travis build as closely as possible. I started with the recommended CI workflow which included doing the build in a subdirectory instead of in-place, and explicitly specifying a build-type. I believe this is currently best-practice, so I thought I'd stick with what they recommend. The only problem I found is the docs build then fails to find the markdown files in the repository root, and I haven't yet figured out how to fix this. Do you have any strong opinions on what is better here? In-place builds are a bit simpler and we inherited them from the old autoconf days, but perhaps it's time to update? Explicitly specifying the build-type is probably also better, and I really want to add tests for Debug builds since it enables all the assertions that could catch problems Release builds miss.

• I think using a separate build directory is better than in place. (In place might work, when the separate one fails.) I started this PR with that, but then I adjusted it to get closer to the travis one. 🙅
• I would run the CI only against release builds. (I usually develop my code with the debug flags, so that is tested enough.)
• It's a fair point to have the debug builds as well, as you said for the assert macros.

6. You are always specifying the compiler to use, whereas I use the platform default compiler unless I explicitly want to test another compiler. In general I really do want to test using the default compiler as the best representative case, but I also want to test using different (clang) compilers. Particularly on non-linux platforms, I'm not even sure what the default compiler is. There is a risk that the default compiler is the same as an explicitly selected compiler so I just test the same compiler twice. Always explicitly specifying the compiler makes the workflow simpler. I tested what happens if compiler="" in the matrix and -D CMAKE_C_COMPILER="" gives a cmake error. In mine I don't have a compiler entry in the matrix and just put the -D CMAKE_C_COMPILER=clang in the options. Thought on what's best here?

As you said, -D CMAKE_C_COMPILER=clang sets the compiler. How do you make that, I don't think it's important... Coming from a matrix or defined explicitly is more of the overall strategy what are the variables of your build. I think the important question you need to answer is: how many different build I need to run to capture the possible bugs? My thoughts are like this:

• Running multiple compiler against the project is advised. (to avoid compiler specific codes)
• Running on all supported OS is advised. (to avoid os specific codes)
• Running test against all build options is advised. (to guarantee all options are in working conditions) The options I refer here is the build options like: -D BUILD_SHARED_LIBS=OFF

I would make a matrix from these, and extend with the special cases if there are any.

Maybe you should try to merge one of the PRs, and see how it works. I think it's hard to prepare for everything in advance. You will see how it works for this project, and can adjust later. If builds gets slower, you turn on caching. If the build fails too many times for strange dependency reasons, you migrate to docker. etc... It's hard to make it right from the beginning. 😄

More questions :-)

1. I see you are getting iwyu by pulling it from git and compiling it. I see that Debian has had an iwyu package for ages, why not just apt-get it?
2. Your iwyu workflow just spits out recommendations and always passes. If we make the code clean, could we make it fail if there are unresolved recommendations?
3. Your clang-tidy workflow is adding to the path and running run-clang-tidy.py, but on my Debian system it seems the clang-tidy package installs /usr/bin/run-clang-tidy so you should be able to just run it without that.
4. Your clang-sa workflow is installing python, pip, and scan-build. On my Debian system installing clang-tools installs scan-build, and scan-build-py-11 in /usr/bin. I'm not sure why analyze-build is missing, but what else does it do? It does seem to be installed at /usr/share/clang/scan-build-py-11/bin/analyze-build, but no symlink for it is added to /usr/bin like the others.
5. The clang-sa output appears to be a subset of the clang-tidy output. Does clang-sa add much that is worth having in addition to clang-tidy?
6. Does clang-tidy add much over the normal clang compiler output? The code is currently pretty clean because I've cleaned up all the compiler warnings from gcc, clang, and even windows. It seems clang-tidy has found some minor things, but in general do you find it a valuable addition?

These tools are pretty cool and I've not used them before. In general I like to use Debian's packages as a vote-of-confidence in the level of maturity/support for a project and for selecting/using reliable releases. If it's not in Debian, it's probably too fringe to use, and using the packaged version ensures I'm using a reliable release that is compatible with the rest of my system. It also makes setup instructions simpler; "just apt-get these". There would have to be pretty compelling reasons to use a more bleeding-edge version.

The iwyu stuff is particularly nice. What it doesn't seem to have is a cmdline/mode where it can fix your code, and only spits out reports. It would be nice to include it as part of the "make tidy" target.

Hey @dbaarda ,

1. I see you are getting iwyu by pulling it from git and compiling it. I see that Debian has had an iwyu package for ages, why not just apt-get it?

I am not a debian (nor ubuntu) user. I was not aware that these tools are packaged well for this platforms. (Was searched for it with duck-duck-go, but have not found them.) If the OS package is available that's easier to use than compile from sources.

2. Your iwyu workflow just spits out recommendations and always passes. If we make the code clean, could we make it fail if there are unresolved recommendations?

I think the tool is silent if it has nothing to do. So, we can fail it on non-empty output.

3. Your clang-tidy workflow is adding to the path and running run-clang-tidy.py, but on my Debian system it seems the clang-tidy package installs /usr/bin/run-clang-tidy so you should be able to just run it without that.

The beauty of linux packages. 😄 It's not in /usr/bin on ubuntu. It failed without modifying the PATH variable.

4. Your clang-sa workflow is installing python, pip, and scan-build. On my Debian system installing clang-tools installs scan-build, and scan-build-py-11 in /usr/bin. I'm not sure why analyze-build is missing, but what else does it do? It does seem to be installed at /usr/share/clang/scan-build-py-11/bin/analyze-build, but no symlink for it is added to /usr/bin like the others.

This is a long story, but the short version is this: scan-build is a Perl script. I did rewrote it in Python. Then tried to merge to upstream. After a few years on Clang mailing list, I gave up on the merge. It is still not a replacement in the Clang project... When I install it with pip it's using my implementation... The packaged one is probably from the Clang repo. (I am surprised that they started to package it.)

scan-build is running the build and running the Clang-SA afterwards. analyze-build runs the Clang-SA only from a compilation database. I think for CI the analyze-build is enough.

5. The clang-sa output appears to be a subset of the clang-tidy output. Does clang-sa add much that is worth having in addition to clang-tidy?

Clang-SA and Clang-tidy is two different tools. The clang-tidy is running checks on the AST, while the Clang-SA builds an execution graph (which costs more in time and memory) and the checks are using that. The Clang-SA will catch you hard to find bugs, wile clang-tidy warns you about good practices. (This is my short explanation, you'll find better ones on their homepage.)

6. Does clang-tidy add much over the normal clang compiler output? The code is currently pretty clean because I've cleaned up all the compiler warnings from gcc, clang, and even windows. It seems clang-tidy has found some minor things, but in general do you find it a valuable addition?

Yes, the code is in pretty good shape. Running these tools just help you to keep it like that. (Specially if there are multiple contributors, when the level of tidiness can vary. 😄 ) In PRs it can be valuable to see if formal requirements of the change is passed or not.

To working with other languages (Java, Kotlin, Rust) I saw that these kind of tools are better integrated into the build system. So, developers can run them regularly. And the CI can fail if these checks are violated... To adopt this practices in C project is much harder, because of dependency management is harder. (Not only the project's dependencies, but the tools are also harder to install.)

I just wanted to show that Clang project provided a good tool set beyond the compiler. These tools can help you to keep the code quality on a level. (I have not even recommended the clang-format, which keeps your code in style 😄 ) But these are also not silver bullets, they won't write you good tests, which I think is more important.

Let me know if you need changes on the PR.

OK, I've split my messy experiment PR against my own fork into a few different PR's for different pieces and merged all the stuff that was not directly github action related. This included #223 and #224.

The stuff that was github action related I've tided up and put into a new PR #225. This only includes the CI "Check" workflow, and it doesn't directly replicate the old travis checks because I've decided to include updating things for cmake best practices and better coverage. I'd appreciate your comments/recommendations. I think it's better and has a few more tweaks than what you have here for the CI workflow, but I'm open to any reasons why you might think I've done it wrong.

What I don't have in my PR is your extra clang-tidy, clang-sa, and iwyu workflows. I do want these, so it would be good if this patch could be updated to incorporate whatever we end up choosing for the CI solution.

I think I want to try these using the existing deb packages for things to see how they compare. I'm not sure what the best way to do that is, as depending on how it goes, it might be better to use these instead.

I think I'll do it by creating another alternative pull request and see how it goes.

After fiddling around with build-analyze and clang-tidy, and digging through docs about them both, I've come to the conclusion that build-analyze is actually a subset of clang-tidy. It appears clang-tidy does have all the static analyser checks, at least according to the lists of checks they both support. The docs also say clang-tidy does clang static analysis checks;

https://clang.llvm.org/extra/clang-tidy/

I've found some old comments online hinting that maybe it didn't work as advertised in the past, but it now appears to be working, as all the things found by analyze-build were found by clang-tidy using the same checks.

It looks like clang-tidy is actually some kind of framework for aggregating all sorts of checks together, and the static analysis checks have now been added under it.

So I'm going to drop the analyze-build checks and just use clang-tidy and iwyu.

Thanks for the updates @dbaarda . It seems that my knowledge is rusted on the Clang project, since I am not reading the dev mailing list. 😄

I've now effectively re-implemented all this in #229 which is now merged, and which included a bunch of other related stuff like adding make targets to run the lint checks locally and updated documentation.

So I'm going to close this PR. Thank you very much for submitting it as it taught me lots. I've credited you in the NEWS.md for the PR's that were inspired/copied from this one.