Skip to content
New issue

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

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

Already on GitHub? # to your account

Run copy_from_upstream and test #1589

Merged
merged 5 commits into from
Oct 30, 2023
Merged

Run copy_from_upstream and test #1589

merged 5 commits into from
Oct 30, 2023

Conversation

baentsch
Copy link
Member

Fixes #1586

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

@baentsch
Copy link
Member Author

@SWilson4 @praveksharma @dstebila Please carefully check this: I'm less than certain that the docs have all been generated correctly. The CI test improvement however definitely is needed.

Copy link
Member

@SWilson4 SWilson4 left a comment

Choose a reason for hiding this comment

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

I'd like to hear @praveksharma's thoughts on the constant-time documentation changes for McEliece and Falcon; if I recall correctly he recently worked on some related issues.

docs/algorithms/sig/sphincs.yml Outdated Show resolved Hide resolved
@baentsch
Copy link
Member Author

Oops -- here's a PR I forgot to mention when we just discussed: @praveksharma @SWilson4 : Do you want to review or shall this be superseded by #1585? Would be totally fine with me. Main thing is #1586 gets fixed (incl. the CI shortcoming addressed in this PR).

@SWilson4
Copy link
Member

Oops -- here's a PR I forgot to mention when we just discussed: @praveksharma @SWilson4 : Do you want to review or shall this be superseded by #1585? Would be totally fine with me. Main thing is #1586 gets fixed (incl. the CI shortcoming addressed in this PR).

I'd rather not try to cram the CI fix into (the already massive) #1585, so on that front I'm OK with proceeding with this PR, once we're satisfied that the documentation is indeed generated correctly. However, once #1585 lands we will be out of sync with PQClean, so the CI fix should break the build. (Or, if this PR lands first, #1585 will be blocked from merging.)

How ought we to deal with this? Ideally. I think we'd resolve #1584, but that might get fairly involved. Still, it will need to be done if idempotence under copy_from_upstream.py is a desired property. We could also do a hacky workaround like disabling the CI check for now, or adding some exceptions to it, but that seems like a recipe for future pain.

@praveksharma
Copy link
Member

I'd like to hear @praveksharma's thoughts on the constant-time documentation changes for McEliece and Falcon; if I recall correctly he recently worked on some related issues.

@SWilson4, copying a comment from another PR over here: classic_mceliece.md wasn't updated along with classic_mceliece.yml in #1552 (presumably I forgot to run update_upstream_alg_docs.py at the time) so these changes are desirable. Ditto with Falcon.

@baentsch
Copy link
Member Author

if idempotence under copy_from_upstream.py is a desired property.

I'd argue idempotence under copy_from_upstream is essential for this project: liboqs is pretty much completely gutted if it cannot reliably import the algorithms. As we're not under any time pressure, I'd argue to do it "right": Fix #1584 first, maybe (just as a test) use that to re-generate #1586 (it seems we now have clarity on the McEliece docs, thanks @praveksharma ), then re-base #1585 (giving the improved copy_from_upstream a final "run for its money" -- and have it focus on HQC and not the other shenenigans).

@baentsch baentsch requested a review from bhess as a code owner October 26, 2023 15:57
@baentsch baentsch requested a review from SWilson4 October 26, 2023 15:58
Copy link
Member

@bhess bhess left a comment

Choose a reason for hiding this comment

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

see single comment above about rmtree.

@bhess bhess self-requested a review October 26, 2023 21:01
Copy link
Member

@bhess bhess left a comment

Choose a reason for hiding this comment

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

Thanks @baentsch for the update and for finding & fixing this bug!
Looks good to me.

@baentsch
Copy link
Member Author

@dstebila Your "code owner" flag apparently overrules 3 people agreeing that this is OK, so please review.

@dstebila
Copy link
Member

@dstebila Your "code owner" flag apparently overrules 3 people agreeing that this is OK, so please review.

I've approved it. I've also changed the branch protection rules on main to require 2 committers to approve, rather than requiring 1 committer and a code owner, since we aren't consistently making use of the code owner functionality.

@baentsch
Copy link
Member Author

@SWilson4 unless I hear howls of protest I'll merge this then and re-base "sw-hqc-pull"/#1585 as I'm already using that branch to test out my ideas for resolving #1584...

@baentsch baentsch merged commit bd943ce into main Oct 30, 2023
39 checks passed
@baentsch baentsch deleted the mb-fixcopyfromupstream branch November 3, 2023 05:30
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

copy_from_upstream run mistake (also in 0.9.0 release)
5 participants