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

do full upstream doc generation [skip ci] #1066

Merged
merged 3 commits into from
Sep 14, 2021
Merged

do full upstream doc generation [skip ci] #1066

merged 3 commits into from
Sep 14, 2021

Conversation

baentsch
Copy link
Member

@baentsch baentsch commented Aug 4, 2021

Fixes #1064.

Will not switch to Ready for Review before @xvzcf looked at the logic changes and @bhess confirmed the contents changes (dropping an author and build opts in Dilithium doesn't seem OK to just leave it to a script to decide). If the changes as per this PR are legit, we may need to re-release 0.7.0.

After possible changes, old update_pqclean_alg_docs.py will be deleted and the new script will be added to copy_from_upstream.py before merge.

  • 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 the list of algorithms available -- either adding, removing, or renaming? (If so, PRs in OQS-OpenSSL, OQS-BoringSSL, and OQS-OpenSSH will also be required by the time this is merged.)

@baentsch baentsch requested review from xvzcf and bhess August 4, 2021 10:26
@bhess
Copy link
Member

bhess commented Aug 4, 2021

dropping an author and build opts in Dilithium doesn't seem OK to just leave it to a script to decide

The author should not be dropped, he is part of the round 3 submission. It seems that he is still missing in the upstream yml-file which could be the cause.

The sse2 and ssse3 dependencies are due to the AES-implementation still bundled with the Dilithium code. The reason the script might have not picked it up is because the upstream-yml defines it in a common_aes dependency.
However, sse2 and ssse3 flags should be redundant when defining avx2, at least GCC makes their intrinsics implicitly available with the -mavx2 flag.
Looking at other schemes, Falcon for example also uses sse2 intrinsics but only sets the avx2 flag.

@baentsch
Copy link
Member Author

baentsch commented Aug 4, 2021

Thanks for the background, @bhess. What's your recommendation then, how to proceed? Wait for an upstream YML-update to get the authors list right? Ensure to load the common.yml to be sure SSE is included -- or drop it as it may be superfluous? Or just keep dilithium.yml and kyber.yml as-is (manually created) and "just" include the new update_upstream_alg_docs.py in this PR -- and run it only at the next copy_from_upstream execution time (assuming all upstream YMLs are corrected by then)?

@bhess
Copy link
Member

bhess commented Aug 4, 2021

@baentsch As discussed, ideal would be to load the flags of the dependencies. At the same time it should be ok to keep the manually created yml for now as the content seems to be ok. Thanks.

@bhess
Copy link
Member

bhess commented Aug 4, 2021

ps: just noticing that also the 'upstream' field in this PR has been changed for kyber and dilithium. These are valid updates compared to the manually created ymls (now it's the commit defined in copy_from_upstream.yml rather than what is defined in the upstream-yml).

@baentsch
Copy link
Member Author

baentsch commented Aug 6, 2021

@bhess Please check the latest file contents: This code now merges 'required_flags' from Common_META (if present) into the OQS doc-YML master files.

@bhess
Copy link
Member

bhess commented Aug 6, 2021

@bhess Please check the latest file contents: This code now merges 'required_flags' from Common_META (if present) into the OQS doc-YML master files.

Thank you for the update! The changes the script made look good to me (except the removed Dilthium author, which will be added to upstream with pq-crystals/dilithium#50).
It's nice that it also catches the dependency flags for Kyber(-90s).

@baentsch
Copy link
Member Author

baentsch commented Aug 6, 2021

Thanks for the re-confirmation that this is logically sound. Regarding contents, well, this is then up to the upstream to put it right. I'll then finalize the PR by integrating it into copy_from_upstream.

@baentsch baentsch marked this pull request as ready for review August 6, 2021 14:35
@baentsch baentsch requested a review from dstebila as a code owner August 6, 2021 14:35
@baentsch
Copy link
Member Author

baentsch commented Aug 6, 2021

This PR now integrates all upstream META information into the liboqs doc YAML files (when running python3 copy_from_upstream.py copy. As per the above, upstream dilithium has an error in the authors list which currently (thus) is replicated into liboqs documentation. @bhess, please do another PR to correct this as and when pq-crystals/dilithium#50 merges. It would have been nice to include this in liboqs 0.7.0 but I wouldn't hold our release for this.

@dstebila
Copy link
Member

dstebila commented Aug 6, 2021

Let's hold this until after 0.7.0 release, I'm hoping that rc4 is the last release candidate.

@baentsch
Copy link
Member Author

baentsch commented Aug 6, 2021

Let's hold this until after 0.7.0 release, I'm hoping that rc4 is the last release candidate.

OK. Will merge only when 0.7.0 is final.

@dstebila
Copy link
Member

Is this ready to merge?

@baentsch
Copy link
Member Author

baentsch commented Aug 18, 2021

Is this ready to merge?

The logic is OK AFAIK, but the data produced by this code currently is not -> If this gets run before pq-crystals/dilithium#50 gets merged, this generates incorrect documentation for our project which I personally find not good: Thinking about it again and as unhappy as I am to not be able to integrate this work, I'd prefer to rather further delay this in line with the upstream (for which this code has been custom-made) than cause our documentation to turn wrong by merging now. Edit: Would it thus be reasonable to revert this to "Draft"? Didn't find the suitable button for this, though...

@baentsch baentsch marked this pull request as draft August 20, 2021 04:49
@bhess bhess mentioned this pull request Sep 13, 2021
2 tasks
baentsch and others added 2 commits September 14, 2021 07:18
added Common_META merge logic

integrated src and doc upstream copy [skip ci]
@baentsch
Copy link
Member Author

Latest commit contains a rebase as well as a fix for correctly updating "upstream" information. Also contained are the results of a re-run of copy_from_upstream (now including doc-YML generation) andupdate_docs_from_yaml: Before merging, I'd suggest

@bhess to re-check all docs (generated) regarding pqcrystals
@xvzcf, time permitting, to check the logic-change pertaining 'upstream' information integration

@baentsch baentsch marked this pull request as ready for review September 14, 2021 08:05
@bhess
Copy link
Member

bhess commented Sep 14, 2021

@bhess to re-check all docs (generated) regarding pqcrystals

Thanks for this update! The docs including the upstream-field look accurate.
Just one minor thing: the order of the required_flags field seems to have changed. For example in Kyber512:

main                 This PR
required_flags:      required_flags:
      - avx2               - avx2
      - popcnt             - bmi2
      - bmi2               - popcnt

I don't see anything related in the script that has changed, but I can imagine that the order of the set() is not unique:

https://github.com/open-quantum-safe/liboqs/pull/1066/files#diff-b12590f74247cf2fc0947d473a38c817dd76e877820d31e56a9a53ae73d7fb34R150
https://github.com/open-quantum-safe/liboqs/pull/1066/files#diff-b12590f74247cf2fc0947d473a38c817dd76e877820d31e56a9a53ae73d7fb34R232

Perhaps the required_flags list could be sorted to have a unique order.

@baentsch
Copy link
Member Author

Perhaps the required_flags list could be sorted to have a unique order.

Good suggestion: Thanks. So done (and one logic error removed). Please check again if you feel like it, @bhess.

@bhess
Copy link
Member

bhess commented Sep 14, 2021

Please check again if you feel like it, @bhess.

Looks good to me. Thank you!

@baentsch baentsch merged commit c0a550f into main Sep 14, 2021
@baentsch baentsch deleted the mb-upstreamdocs branch September 22, 2021 04:48
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.

Enhance doc generation to use all upstream sources & integrate to copy_from_upstream
3 participants