Fix incorrect formatting in unix.yml

[praveksharma]

An incorrect merge with main in #1745 broke the formatting in .github/workflows/unix.yml which was not caught before the PR was merged causing some CI workflows to not run; this fixes that.

[No] Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
[No] Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged.)

[baentsch]

The fix is OK. Would we want to learn from/improve things going forward regarding things like this? What about creating an issue asking for a CI job linting all CI YMLs such as to not depend on GH highlighting (possibly not doing that visibly enough)?

[SWilson4]

Looking at https://github.com/open-quantum-safe/liboqs/actions/runs/10517373588, it seems like only two "linux" jobs completed. This is confusing... maybe the libjade-build variable is overriding the rest of the matrix configuration?

[baentsch]

Looking at https://github.com/open-quantum-safe/liboqs/actions/runs/10517373588, it seems like only two "linux" jobs completed. This is confusing... maybe the libjade-build variable is overriding the rest of the matrix configuration?

Yikes -- good catch -- I only looked in awe at already 114 checks (as opposed to apparently 24 in "main") -- but indeed, if the idea is that the 2 libjade tests should "mix" with the other 14 linux "include" settings, is the idea that this should be 28 more tests -- or 2 more tests? If the latter, shouldn't the libjade tests be 2 separate entries in the "include" list?

[baentsch]

Thanks for the fix @praveksharma ! At the risk of making myself ridiculous by retaining my already given approval despite the previous failure, let me do that regardless.

Beyond the suggestion above (1) to create an issue checking CI tasks, some more questions/observations:
2) This PR seems to add 5 more test configs for specific libjade-enabled configs to the previous 14 "linux" configs. The latest CI run accordingly shows 19 tests (addressing @SWilson4 's observation of the previous run's 2 (libjade-only) runs). But the overall count of "checks" goes from the already astounding 114 to 143, not to what I'd expect 114+19-2=131: What further (12) checks did this change trigger? What do I overlook (or is there something else fishy)?
3) When looking at the full CI report, it shows a very long list of warnings: Is this something OQS did knowingly accept or another area for improvement (another issue?)?

[praveksharma]

What about creating an issue asking for a CI job linting all CI YMLs such as to not depend on GH highlighting (possibly not doing that visibly enough)?

Quoting @SWilson4 from #1745 (comment)

I think this will be improved by #1880.

#1880 is working towards the solution you're suggesting here @baentsch.

[praveksharma]

2. This PR seems to add 5 more test configs for specific libjade-enabled configs to the previous 14 "linux" configs. The latest CI run accordingly shows 19 tests (addressing @SWilson4 's observation of the previous run's 2 (libjade-only) runs). But the overall count of "checks" goes from the already astounding 114 to 143, not to what I'd expect 114+19-2=131: What further (12) checks did this change trigger? What do I overlook (or is there something else fishy)?

The GH actions UI reads "1 skipped and 147 successful checks" after ec805bd and "1 skipped and 113 successful checks" after 3b0f290. Since each check is run twice (once on push to the branch and once on pull request) the numbers seem to add up with 114 - (2 * 4) + (2 * 19) = 148. I'm not sure why you're seeing 131; does the GH actions UI list different numbers for us?

[praveksharma]

3. When looking at the full CI report, it shows a very long list of warnings: Is this something OQS did knowingly accept or another area for improvement (another issue?)?

Focusing on warnings related to runs in unix.yml, I'm seeing 43 warnings. 3 related to actions using a deprecated version of Node.js. The other 40 (20 each) are homebrew warnings regarding using an older gcc version and using "--ignore-dependencies" option, both enabled by #1805

[praveksharma]

Looking at https://github.com/open-quantum-safe/liboqs/actions/runs/10517373588, it seems like only two "linux" jobs completed. This is confusing... maybe the libjade-build variable is overriding the rest of the matrix configuration?

Thank you pointing this out @SWilson4!

if the idea is that the 2 libjade tests should "mix" with the other 14 linux "include" settings, is the idea that this should be 28 more tests -- or 2 more tests? If the latter, shouldn't the libjade tests be 2 separate entries in the "include" list?

The idea was the former, but I'd misinterpreted GH actions documentation regarding how the standard matrix key:pair values are augmented by those in the include list. This would still have been broken despite the incorrectly formatted YAML (which is hopefully fixed by the actionlint PR). Part of the reason this got masked is because the additional macos tests worked as expected (the matrix config for these doesn't use an include list) and I eyeballed the extra checks instead of verifying increased check numbers. Going ahead, arithmetic checks seem like a good rule of them when modifying CI files.

[praveksharma]

@baentsch @SWilson4 what are you thoughts on disabling automatic checks on push to a branch? This ought to clean up the GH actions UI for PRs a little bit. These checks can still be run manually via the workflow_dispatch option via the GH CLI.

[baentsch]

Since each check is run twice (once on push to the branch and once on pull request)

Thanks for the statement: It's showing my mistake in thinking: I assumed CI was set up in line with open-quantum-safe/tsc#5 and the same test runs only once :-(

@baentsch @SWilson4 what are you thoughts on disabling automatic checks on push to a branch? This ought to clean up the GH actions UI for PRs a little bit. These checks can still be run manually via the workflow_dispatch option via the GH CLI.

Hmm -- I actually wonder whether running the same tests on PR should not be cut down: Additional tests, Yes, the same ones, No. AFAIK there's no way a PR can be started without first having done a push -- and all CI results (push and additional PR ones) should be shown in the UI in the PR "Checks" tab, no?

#1880 is working towards the solution you're suggesting here @baentsch.

Thanks also for that pointer: Great! Then let's get that merged and combined with a statement in CONTRIBUTING.md that no PRs shall ever be merged if that test fails (and only in exceptional circumstances on other CI failures).

[SWilson4]

Thanks for fixing this @praveksharma!

@baentsch @SWilson4 what are you thoughts on disabling automatic checks on push to a branch? This ought to clean up the GH actions UI for PRs a little bit. These checks can still be run manually via the workflow_dispatch option via the GH CLI.

I'm fine with this. If it's possible to keep the basic checks (style, upstream, build) that might be nice.

Hmm -- I actually wonder whether running the same tests on PR should not be cut down: Additional tests, Yes, the same ones, No. AFAIK there's no way a PR can be started without first having done a push -- and all CI results (push and additional PR ones) should be shown in the UI in the PR "Checks" tab, no?

I'd have to read the fine print in GitHub documentation and/or test it, but I think we need all the tests to run on PRs in order to ensure that contributions coming from forks are fully tested.

[baentsch]

I think we need all the tests to run on PRs in order to ensure that contributions coming from forks are fully tested.

That indeed may be a reason. But isn't there a way then to avoid tests in pushes&PR's from us/authored on openquantumsafe (I still think the majority of all PRs) to be run only once?

[SWilson4]

I think we need all the tests to run on PRs in order to ensure that contributions coming from forks are fully tested.

That indeed may be a reason. But isn't there a way then to avoid tests in pushes&PR's from us/authored on openquantumsafe (I still think the majority of all PRs) to be run only once?

I think adopting @praveksharma's suggestion of disabling automatic runs on push (except to main) would accomplish this nicely: the full tests will run once on PRs, including from forks, but not on pushes to development branches, unless manually triggered. This would also (imo) get rid of a lot of unnecessary runs on pre-PR branches. For example, I sometimes push in-progress work to have it backed up and don't always remember to add [skip ci]).

[baentsch]

unless manually triggered.

Agree -- if that (procedure, i.e., that contributors need to trigger pre-PR CI manually) is then clearly documented in CONTRIBUTUNG.md as it may be unusual for the "unwary".