Add more CI tests

[jkbonfield]

These are broken into two styles:

• qemu based VMs running FreeBSD, OpenBSD and OpenSolaris.
• Container based images running a variety of other linux distributions.

Apart from one image, none are using address sanitizer. (We could disable it if we wanted on the FreeBSD, but I thought one single image doing that with a very different OS design may be helpful.)

They should all be reasonably quick to run. I also updated the INSTALL file with a couple other machines and tweaked things minimally elsewhere.

See also https://github.com/jkbonfield/c-maint/actions/runs/29008485376 for an example combined workflow for samtools+htslib and bcftools+htslib within c-maint (PR to follow). That will likely be on cron, to periodically validate building and testing cross-compatibility between repos.

[jkbonfield]

TODO based on copilot review:

• Use commit pinning for the community VM projects, as they're not officially GitHub maintained.

• Consider dropping archlinux here and switching opensuse to leap. Currently those two are rolling distros, which was a deliberate choice, but it may also mean we have CI breakages. We could "fix" that by temporarily disabling them if required, or we could make the CI validation work on a cron (eg in c-maint) so we get periodic validation but don't break on-push/commit CI. I think the former is useful though: knowing a rolling release broke something may be a forewarning of something that will affect more mainstream releases down the line (or it could just be a bug).

[daviesrob]

For the VM builds, should we use cache-after-prepare: true? It would reduce the number of package updates, which might be appreciated by some of the less well resourced distributions that we're cloning. It would only work if the total cached size is less than Github's limit (10Gb, apparently). The main disadvantage would be that the package updates would become sticky, unless we can think of a way of evicting them every so often.

Similarly, it might be good to work out a way of caching the containers with the installed dependencies. In theory that could be done by pushing the updated containers to quay.io or docker hub, and then using the enhanced one instead of the original. An advantage would be less risk of jobs failing to due package manager issues before we've even got to running the HTSlib tests.

[daviesrob]

I think we should remove -std=c99 -pedantic. This combination is covered by other tests, and isn't how the package would normally be built.

[jkbonfield]

I just inherited it from whatever one I used as a base line in the linux-build. I had it to try and spot extra compatibility issues, as these are limiting the compiler (given gnu99 is probably more the default these days) rather than expanding it.

However yes, we should probably just be using whatever the system has as the defaults out of the box given we don't have explicit compiler options in our INSTALL notes. I'll fix this.

[daviesrob]

We should really work out how to adjust the code so these settings aren't needed.

[jkbonfield]

They may go away if I remove the -std=c99 -pedantic options. If I recall it was mostly musl issues and system include files not strictly complying with the additional warning levels. I'll have a play. However given -Wall isn't the default compiler option anyway, doing -Wall -Wno-xyz isn't so bad.

[jkbonfield]

So here's the first tranch of failures. Do we really want to fix this in the code by adding unnecessary casts everywhere? I'm more incline to say we should be minimally adding build instructions in INSTALL (for systems that are vanishingly likely to be used) or if it's something regular adding it to autoconf, which is more work but definitely justified on regular platforms.

kstring.c: In function ‘boyer_moore’:
kstring.c:468:24: error: return discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
  468 |                 return memchr(str, pat[0], n);
      |                        ^~~~~~
faidx.c: In function ‘fai_path’:
faidx.c:1067:25: error: initialization discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
 1067 |         char *fai_tmp = strstr(fa, HTS_IDX_DELIM);
      |                         ^~~~~~

Techically the first one is down to static void *boyer_moore instead of static const void *boyer_moore, but changing that may expose weaknesses elsewhere, including in other peoples code. We could do it obviously and it's correct, but it's changing a public API albeit subtly.

The second one is a library implementation thing. The strstr prototype is char *strstr(const char *haystack, const char *needle);. It ought to be const char * given it's not modifying the string, but it's not. However I expect this library has a static inline somewhere so the compiler is able to track everything through and spot the loss of const. Yes we could add an explicit cast, but it's littering the code with noise that's basically just a box-ticking exercise. We could either do that, or just accept it's a known library defect and silence it globally. The pragmatist in me says the second option is far less work and it's a priority thing (there are bigger wins to be had elsewhere with our time IMO).

[jkbonfield]

Alpine hits the ref-cache issue we've seen elsewhere:

ref_cache/cmsg_wrap.c:71:51: error: unsigned conversion from 'long int' to 'long unsigned int' changes value from '-8' to '18446744073709551608' [-Werror=sign-conversion]
   71 |     for (cmsg = CMSG_FIRSTHDR(msg); cmsg != NULL; CMSG_NXTHDR(msg, cmsg)) {
      |                                                   ^~~~~~~~~~~

Again it's a libmusl thing. What's the correct fix? The prototype is:

struct cmsghdr *CMSG_FIRSTHDR(struct msghdr *msgh);

That's all structure pointers - no longs anywhere. So it's almost certainly something internal to a system header file with static inline code. The real problem isn't the code, but the overly ambitious compiler command lines. We don't use just gcc to build this, but gcc -Wall -g -O3 -fvisibility=hidden -Werror -Wextra -Wformat -Wformat=2 -Wconversion -Wtrampolines -Wstrict-aliasing -fstack-clash-protection -fstack-protector-strong -fcf-protection=full -fno-delete-null-pointer-checks -I. -c -o ref_cache/cmsg_wrap.o ref_cache/cmsg_wrap.c. We've basically added a whole raft of unnecessary naval gazing and are now paying the price, so this is simply an attempt to unwind some of that. Maybe we should globally add -Wno-sign-conversion to the configure/Makefile rules for ref_cache so it's not triggering system internal non-bugs.

[jkbonfield]

For the VM builds, should we use cache-after-prepare: true? It would reduce the number of package updates, which might be appreciated by some of the less well resourced distributions that we're cloning. It would only work if the total cached size is less than Github's limit (10Gb, apparently). The main disadvantage would be that the package updates would become sticky, unless we can think of a way of evicting them every so often.

It's possibly something we can do differently between the regular repo builds on PR, commit, etc and the weekly c-maint one, as that gives us coverage of the latest package changes and warning of incompatibilities, while speeding up other things.

Similarly, it might be good to work out a way of caching the containers with the installed dependencies. In theory that could be done by pushing the updated containers to quay.io or docker hub, and then using the enhanced one instead of the original. An advantage would be less risk of jobs failing to due package manager issues before we've even got to running the HTSlib tests.

I thought about that, but it's another layer of extra complexity that I haven't researched yet and in the general direction of being requested to not put too much into each PR I decided such improvements can occur in subsequent PRs.

[jkbonfield]

Well... this is an intriguing one: https://github.com/samtools/htslib/actions/runs/30342083343/job/90219742456?pr=2053

test/test_ksort.c: In function ‘main’:
test/test_ksort.c:230:19: error: implicit declaration of function ‘getopt’ [-Wimplicit-function-declaration]
  230 |     while ((opt = getopt(argc, argv, "a:bhn:")) != -1) {
      |                   ^~~~~~

It's correct of course - POSIX getopt is in unistd.h, not getopt.h. That's a GNUism I think.

However... test/test_ksort.h isn't in this PR! It's in develop and was added recently, but I hadn't rebased it. Very odd.
The commit hash it's pulling down doesn't seem to exist, so I wonder if it's doing a local merge to develop and testing the merged PR rather than the PR as stands. Curious and something to investigate more as it's certainly not what I expected to happen.

Edit: yes it auto-merges. If we wish to block it, I think we can add:

  with:
    ref: ${{ github.event.pull_request.head.sha }}

However is it a useful feature we wish to keep?