-
Notifications
You must be signed in to change notification settings - Fork 480
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
Conversation
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. |
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 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., 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:
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. |
There was a problem hiding this 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 !
There was a problem hiding this 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.
It seems that we (i.e., anyone with write perms) can do this via the GH API. I've just added the |
Setting a workflow to run on
Done, and happy to do a show and tell any time. |
bea388f
to
fbfa917
Compare
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]>
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]>
…rigger downstream] Signed-off-by: Spencer Wilson <[email protected]> Signed-off-by: Spencer Wilson <[email protected]>
Withdrawing approval as apparently out-of-scope changes to suppression files landed
Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Spencer Wilson <[email protected]>
a0ef386
to
bbec489
Compare
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]>
This reverts commit 1305d50. Signed-off-by: Spencer Wilson <[email protected]>
bbec489
to
a8d4721
Compare
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 I removed unsupported downstream projects from the commit-to-main workflow, and added liboqs-go and liboqs-cpp. |
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. |
There was a problem hiding this 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.
Fixes #1780, fixes #1783.