Refactor CI

[Joseph-Edwards]

Edit

This goal of this PR has changed. Please refer to #641 (comment) for an updated idea of the changes this PR introduces.

Original post

This PR:

• Creates an action for running gap tests with code prepended to the gap command, allowing for GAP to be run with Valgrind (amongst other things).
• Creates a Valgrind workflow
• Creates a pull request workflow with workflow dependencies
• Allows workflows to be dispatched with custom inputs.

The dependency structure is:

1. Standard tests pass
2. Lint, Codespell and Manual pass
3. External planarity bliss, Cygwin, Extreme tests and Valgrind pass

Note that, presently, these changes depend on the workflows files in my fork. This is because, prior to this PR, those files didn't exist in the Digraphs fork. When this is merged, I will resolve this.

Also note that the new action created in this PR is very similar to an existing gap action. I've opened a pull request into that action, and if that is merged I will remove the custom action from this repository.

[james-d-mitchell]

Closing and re-opening to see if that fixes the Ci reporting (i note that the ci actually passed, but isn't reported as such on this page for some reason).

[james-d-mitchell]

Ah no wait the reported "pending" jobs are set to "required" for PR's and so show up even though they no longer exist, my bad. Any idea why the jobs in "second" aren't shown here @Joseph-Edwards ?

[Joseph-Edwards]

I'm not certain, but I think it's because none of the jobs from "second" are defined in the pr.yml file. So we wouldn't expect anything to appear as pending until "first" has finished. When first has finished, the "second" job begins, and that's when it discovers all of the sub-jobs. I don't have anything to back up this claim, but that looks like what's happening to me.

[james-d-mitchell]

While we're at it, can we transfer the azure jobs to gh actions?

[james-d-mitchell]

I think I'll be happy to merge this with a couple of caveats:

• hard coded paths to your repos will have to be replaced (I understand why they are there just now)
• is it possible to remove the prefix "PR" from the listed jobs? It's completely redundant, and not very informative
• can you please add some sort of README to the .github/workflows directory that explains how the yml files are organised? It's not completely clear to me why for example, gap_standard.yml is in a separate file, and why first_group.yml isn't just contained in pr.yml etc. i.e. why do some of the workflows have their own file while others are part of the "aggregate" files?

[james-d-mitchell]

Also requires to be rebased after I've merged: #642

[Joseph-Edwards]

I think I'll be happy to merge this with a couple of caveats:

* hard coded paths to your repos will have to be replaced (I understand why they are there just now)

* is it possible to remove the prefix "PR" from the listed jobs? It's completely redundant, and not very informative

* can you please add some sort of README to the `.github/workflows` directory that explains how the yml files are organised? It's not completely clear to me why for example, `gap_standard.yml` is in a separate file, and why `first_group.yml` isn't just contained in `pr.yml` etc. i.e. why do some of the workflows have their own file while others are part of the "aggregate" files?

I'll look into this and make the necessary changes. The reason that some of the jobs have their own file, and some of them don't is because I wanted to see what would happen if that was the case. I'll do some restructuring to try and make everything more rational.

[james-d-mitchell]

@Joseph-Edwards and I discussed this just now, and essentially decided that the approach taken here is too complicated (for @james-d-mitchell at any rate).

We will do the following workflows:

1. Ubuntu / GAP master / required or only-needed + Ubuntu / GAP stable-4.X (using the all required packages for Digraphs)
2. Only needed / GAP master + Ubuntu / GAP stable-4.X (combine with one if can be done in a matrix)
3. os / cygwin/ubuntu32/macosx
4. config-options / with-external-planarity etc
5. Linting
6. Manual
7. Codespell

[Joseph-Edwards]

Temporarily closing this while testing to avoid cluttering the actions triggered by pull requests

[Joseph-Edwards]

These changes address #641 (comment).

[Joseph-Edwards]

Presently, the config-options workflow tests all non-empty combinations of enable-code-coveragexenable-compilexenable-debugxwithout-intrinsics + with-external-planarityxwith-external-bliss for a total of $2^4 + 2^2 - 2 = 18$ options. This was decided because the planarity and bliss options require a conda environment, so they were considered separately. If this is undesirable, we can test all $2^6 - 1 = 63$ options, and build a conda environment for each. With caching, I don't think that would add too much overhead.

[Joseph-Edwards]

As the comment says, has NautyTracesInterface been excluded purposefully?

[james-d-mitchell]

I don't think it was excluded on purpose, the package works with and without it, the tests test with and without it enabled.

[Joseph-Edwards]

When we aren't explicitly testing for compatibility with a version of GAP, which branch should we use? Here we use master, but in config_options.yml we use stable-4.13.

[james-d-mitchell]

I think it makes sense to check against stable-4.13, since this should be stable at least :)

[Joseph-Edwards]

Most (but, surprisingly, not all) of the --enable-debug tests seem to fail. Does GAP need to be built with specific config options for this to work, or is this an issue with Digraphs?

[james-d-mitchell]

Most (but, surprisingly, not all) of the --enable-debug tests seem to fail. Does GAP need to be built with specific config options for this to work, or is this an issue with Digraphs?

Looks like issue in digraphs, will fix first thing

[james-d-mitchell]

Maybe we should add the CFLAG/CXXFLAG that indicates that a warning should be treated as an error in the jobs that have --enable-compile-warnings if any? @Joseph-Edwards

[Joseph-Edwards]

Maybe we should add the CFLAG/CXXFLAG that indicates that a warning should be treated as an error in the jobs that have --enable-compile-warnings if any? @Joseph-Edwards

I've tried this locally with

./configure --enable-compile-warnings WARNING_CXXFLAGS="-Werror" WARNING_CFLAGS="-Werror"

as standard CXXFLAGS and CFLAGS didn't seem to work. The problem with this is that some of the warnings seem to be emanating from GAP itself, so error-ing out on these doesn't make sense to me.

How do you think we should proceed @james-d-mitchell?

An example

In file included from /home/joseph/Dev/Comp_Maths/gap/src/gap_all.h:21,
                 from /home/joseph/Dev/Comp_Maths/gap/src/compiled.h:21,
                 from src/gap-includes.h:23,
                 from src/digraphs.h:19,
                 from src/digraphs.c:15:
/home/joseph/Dev/Comp_Maths/gap/src/ariths.h: In function ‘EQ’:
/home/joseph/Dev/Comp_Maths/gap/src/ariths.h:271:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
  271 |     UInt tnumL = TNUM_OBJ(opL);
      |     ^~~~

[Joseph-Edwards]

Temporarily closing again while testing.

[james-d-mitchell]

Rebased after merging #648

[james-d-mitchell]

Any reason not to merge this @Joseph-Edwards ? Looks good to me now!

[Joseph-Edwards]

Looks good to me too @james-d-mitchell. Happy to merge.

[james-d-mitchell]

Thanks!