Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor liboqs CI and update Ubuntu images #1909

Merged
merged 41 commits into from
Sep 9, 2024
Merged

Refactor liboqs CI and update Ubuntu images #1909

merged 41 commits into from
Sep 9, 2024

Conversation

SWilson4
Copy link
Member

@SWilson4 SWilson4 commented Aug 27, 2024

Fixes #1780, fixes #1783.

@SWilson4
Copy link
Member Author

As a TODO, the Noble image still needs to be used instead of Focal in the Linux tests. In the meantime, please feel free to take a look at the refactor @baentsch @dstebila @praveksharma and let me know what you think. Everything is (hopefully) documented in the new CI.md file.

@dstebila
Copy link
Member

This looks really nice, thanks Spencer! Overall the organization looks very good. I didn't check whether all the tests are reflected in the refactor.

One question I had is what workflow_call and workflow_dispatch mean.

The only other comment I had is that in the new CI.md file (which is great, thanks!) it might be useful to have the subsection headers include descriptive text analogous to what you use inline (e.g., #### Platform tests (platform.yml)) not just the filename.

Also might be good in a future OQS status call to give a brief demo of how to actually trigger a workflow in the Github Actions UI, so that our core developers know how to do that.

@baentsch
Copy link
Member

Also might be good in a future OQS status call to give a brief demo of how to actually trigger a workflow in the Github Actions UI, so that our core developers know how to do that.

@dstebila I just wrote a comment about more and more stuff happening in meetings as wrong:

  1. Not everyone can be in every meeting (or stay to the end)
  2. Meetings tend to digress
  3. Not everything from a meeting is remembered by heart
  4. Things in meetings can get misunderstood

I see the benefit of meetings as alleviating time-pressed people of the need to follow community discussions, but this leads to potentially disregarding relevant thoughts or not capturing relevant knowledge to posterity/the (hopefully) coming new team members.

Thus: Please see meetings only as an extension of what we document for the whole community -- or in concrete terms for this ask to @SWilson4 : Please document a sample command to manually trigger relevant tests documented in CI.md in CONTRIBUTING.md; a demo in a meeting would be nice, too.

baentsch
baentsch previously approved these changes Aug 28, 2024
Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All LGTM -- particularly nice all comments explaining exceptions (libjade not running on aarch64 MacOS14 for example). Generally, the logic & interdependence of testing is much better understandable now: Thanks @SWilson4 !

Copy link
Member

@praveksharma praveksharma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work @SWilson4! Everything looks good to me. Could you also update file names (unix.yml -> linux.yml and weekly.yml -> extended.yml) in the top comment for scripts/copy_from_upstream/copy_from_libjade.yml?

I'd intended to have the LIBJADE_ALG_LIST variable stored at the repo level (as described here: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#creating-configuration-variables-for-a-repository). This might a nice place to make that change if you're able to do that @dstebila. However, this can be done later, I wouldn't want to block progress on this PR.

@SWilson4
Copy link
Member Author

Thanks for the work @SWilson4! Everything looks good to me. Could you also update file names (unix.yml -> linux.yml and weekly.yml -> extended.yml) in the top comment for scripts/copy_from_upstream/copy_from_libjade.yml?

I'd intended to have the LIBJADE_ALG_LIST variable stored at the repo level (as described here: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#creating-configuration-variables-for-a-repository). This might a nice place to make that change if you're able to do that @dstebila. However, this can be done later, I wouldn't want to block progress on this PR.

It seems that we (i.e., anyone with write perms) can do this via the GH API. I've just added the LIBJADE_ALG_LIST repository variable for liboqs and updated the comment in copy_from_libjade.yml accordingly.

@SWilson4
Copy link
Member Author

One question I had is what workflow_call and workflow_dispatch mean.

Setting a workflow to run on workflow_call allows other workflows to trigger it (in their own context) with the uses keyword. Setting a workflow to run on workflow_dispatch allows the workflow to be triggered via the GitHub API (including in the web UI). So, setting (for example) macos.yml to be triggered on workflow_call and workflow_dispatch allows us to trigger it both automatically using caller workflows (such as pr.yml) and manually using the web UI.

The only other comment I had is that in the new CI.md file (which is great, thanks!) it might be useful to have the subsection headers include descriptive text analogous to what you use inline (e.g., #### Platform tests (platform.yml)) not just the filename.

Also might be good in a future OQS status call to give a brief demo of how to actually trigger a workflow in the Github Actions UI, so that our core developers know how to do that.

Done, and happy to do a show and tell any time.

Signed-off-by: Spencer Wilson <[email protected]>

Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Spencer Wilson <[email protected]>

Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Spencer Wilson <[email protected]>

Signed-off-by: Spencer Wilson <[email protected]>

Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Spencer Wilson <[email protected]>

Signed-off-by: Spencer Wilson <[email protected]>

Signed-off-by: Spencer Wilson <[email protected]>

Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Spencer Wilson <[email protected]>
@baentsch baentsch dismissed their stale review September 4, 2024 13:50

Withdrawing approval as apparently out-of-scope changes to suppression files landed

@SWilson4 SWilson4 force-pushed the sw-ci-usage branch 2 times, most recently from a0ef386 to bbec489 Compare September 4, 2024 16:34
@SWilson4
Copy link
Member Author

SWilson4 commented Sep 4, 2024

From my point of view, this is now ready for final review and merge.

A number of new constant-time failures appeared with the new OS / compiler versions. They were all in Falcon (generic) and McEliece (AVX2), both of which already had constant-time "issues", so I classified them all as issues. Almost all seemed to correspond to previously noted issues, shifted by one line number.

One other notable failure: Dilithium and ML-DSA fail the leak tests with the newest clang version on Ubuntu Noble. Based on the error message, I believe this is a Valgrind issue, not an actual memory leak. The tests pass for all algorithms with clang on Ubuntu Jammy. I have included both a noble-clang job which skips the leak tests for Dilithium/ML-DSA and a jammy-clang job.

I also fixed some string syntax in one of our Python scripts and an fclose issue in one of the test files. These were picked up by the newer versions of Python and scan-build, respectively.

I removed unsupported downstream projects from the commit-to-main workflow, and added liboqs-go and liboqs-cpp.

@SWilson4 SWilson4 marked this pull request as ready for review September 4, 2024 18:41
@SWilson4
Copy link
Member Author

SWilson4 commented Sep 4, 2024

I should also mention that I enabled concurrency groups for the push and PR workflows. This means that in-progress push (resp. PR) runs will be cancelled in the event of a new push (resp. commit added to PR). I thought this was in line with responsible CI usage.

Also note that the only duplicate tests on this PR are the most basic ones (formatting, / copy from upstream / minimal build). This is a significant improvement over our existing setup, which duplicates everything.

Copy link
Member

@praveksharma praveksharma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work @SWilson4! I haven't gone through the Falcon and McElliece constant time issues but the changes/additions to CI and documentation look good to me.

@SWilson4 SWilson4 merged commit b37c937 into main Sep 9, 2024
78 checks passed
@SWilson4 SWilson4 deleted the sw-ci-usage branch September 9, 2024 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove unnecessary downstream CI Update Ubuntu support to more current LTS version(s)
4 participants