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

Ensure generic OQS_OPT_TARGET in weekly CT tests #1618

Merged
merged 1 commit into from
Dec 6, 2023
Merged

Conversation

SWilson4
Copy link
Member

@SWilson4 SWilson4 commented Dec 1, 2023

Set -DOQS_DIST_BUILD=OFF in the "generic" weekly constant-time test run so that the OQS_OPT_TARGET variable has an effect. This currently isn't being done, which means that only the AVX2 versions are being tested (in the weekly "extensions" run). As mentioned in #1617.

  • 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.)
  • 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 and OQS-OpenSSH will also need to be ready for review and merge by the time this is merged.)

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.

Conceptually I agree that we should test the generic/reference/C code for CT, so this PR makes sense. But I wonder whether it should be extended to also (keep) testing the optimized code? It's running most of the time (as say AVX2 is available in most machines these days). See #1619 as a possible next step.

@SWilson4
Copy link
Member Author

SWilson4 commented Dec 4, 2023

Conceptually I agree that we should test the generic/reference/C code for CT, so this PR makes sense. But I wonder whether it should be extended to also (keep) testing the optimized code? It's running most of the time (as say AVX2 is available in most machines these days). See #1619 as a possible next step.

The optimized code will still be tested via the extensions job (I think). Right now we have two jobs running the AVX2 code; this change should make it so that one runs the "pure" C code and the other runs the code with extensions.

@baentsch
Copy link
Member

baentsch commented Dec 4, 2023

The optimized code will still be tested via the extensions job (I think)

Ah, Yes. Completely overlooked that one. Indeed "haswell" has the relevant extensions. Thanks (for PR & explanation)!

@baentsch
Copy link
Member

baentsch commented Dec 6, 2023

@dstebila Weekly runs are on Friday, so approval & merge before would be great. Alternatively, shall we exclude such "infrastructure" PRs from the "2 approvals needed" rule?

@dstebila
Copy link
Member

dstebila commented Dec 6, 2023

@dstebila Weekly runs are on Friday, so approval & merge before would be great. Alternatively, shall we exclude such "infrastructure" PRs from the "2 approvals needed" rule?

I don't know how to change the "2 approvals needed" rule to only apply to certain directories -- we can apply it only to certain branches, but I don't see directories as an option.

@baentsch
Copy link
Member

baentsch commented Dec 6, 2023

I don't know how to change the "2 approvals needed" rule to only apply to certain directories -- we can apply it only to certain branches, but I don't see directories as an option.

Hmm -- just read the documentation and I also didn't find a way how to do that. A bit disappointing, I'd say. But what I did find is an option that demands approval by code owners. And that can be set to specific directories, right? So what about the idea to carefully define code ownership(s), set this flag and revert back to just 1 review needed for PRs? Not ideal if there's a sole code owner designated for some sections and that person isn't around -- in such case the 4 eyes rule would be better again...

@SWilson4
Copy link
Member Author

SWilson4 commented Dec 6, 2023

I've preserved @baentsch's suggestion in #1623 in case we want to continue discussion somewhere active after this is merged.

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.

3 participants