add cpp linking test #1963
add cpp linking test #1963
Conversation
Signed-off-by: Aiden Fox Ivey <[email protected]> Signed-off-by: Ry Jones <[email protected]>
|
Hi @aidenfoxivey I moved the code to this PR |
|
I believe this is a good contender for a "how many PRs does it take an engineering student to complete a 2 file change" joke lol. @ryjones Thank you so much for your help. |
.github/workflows/basic.yml
Outdated
| runs-on: ubuntu-latest | ||
| container: openquantumsafe/ci-ubuntu-latest:latest | ||
| env: | ||
| KEM_NAME: ml_kem_768 |
There was a problem hiding this comment.
Do we need this environment variable? I think we only need the SIG_NAME for this test.
There was a problem hiding this comment.
No, don't believe we do - I had left it in to be cautious, but it shouldn't be necessary.
.github/workflows/basic.yml
Outdated
| cd build && \ | ||
| cmake -GNinja -DOQS_STRICT_WARNINGS=ON \ | ||
| -GNinja \ | ||
| -DOQS_MINIMAL_BUILD="KEM_$KEM_NAME;SIG_$SIG_NAME" \ |
There was a problem hiding this comment.
| -DOQS_MINIMAL_BUILD="KEM_$KEM_NAME;SIG_$SIG_NAME" \ | |
| -DOQS_MINIMAL_BUILD="SIG_$SIG_NAME" \ |
There was a problem hiding this comment.
Yep, this makes sense.
Signed-off-by: Ry Jones <[email protected]>
|
One thing I had considered, was redirecting output from running the binary, but I think it's better to keep the trace on stderr for debugging. |
| */ | ||
| static OQS_STATUS example_stack(void) { | ||
|
|
||
| #ifdef OQS_ENABLE_SIG_dilithium_2 |
There was a problem hiding this comment.
Hmm -- I'm not sure I understand this: CI seems to disable Dilithium2 and this test basically only runs when Dilithium2 is enabled, so what is actually tested in CI @aidenfoxivey @ryjones ?
There was a problem hiding this comment.
Hmm, this is a good catch - it should probably be modified to fix this. I don't have access to the repository to make a change right now.
There was a problem hiding this comment.
@aidenfoxivey propose a change in a review comment
There was a problem hiding this comment.
I don't have access to the repository to make a change right now.
Why can't you do a proper PR from your own repo @aidenfoxivey ? Proceeding with this PR would make @ryjones the author of record -- which simply is wrong on several counts.
If the problem is that no reviewer/committer presses the "execute CI" button on a PR, well, then this project has some serious problem of getting enough attention...
@aidenfoxivey if in turn you have problems with the contribution process (where actually the way how to run CI also locally is described) please let us know as part of your PR: We want to improve our documentation such that new contributors can easily, well, contribute.
There was a problem hiding this comment.
@baentsch the issue was that the PR from his repo cannot execute all of the CI due to no access to secrets.
There was a problem hiding this comment.
@ryjones I see at least two other solutions to this problem: 1) improve the CI process such as to remove this limitation: anyone should be able to do a PR and see most-if-not-all CI run (to then fix him/herself); 2) execute CI locally (as is documented).
Working around problem 1) by taking ownership of a PR is kind but neither scalable (you cannot handle by manually pasting in review comments by dozens of contributors having the same problem) nor "right" (e.g., code authorship attribution should be easy to infer both from a legal and ethical perspective).
So, would it be OK for you to make a proposal to improve 1) @ryjones as this seems to be related to admin aspects? I (or maybe @SWilson4?) could work with @aidenfoxivey to make sure this contribution "gets in" with proper attribution. OK for everyone?
There was a problem hiding this comment.
@baentsch It isn't obvious, but I did do non-zero work on this PR.
As far as token availability, this is a limitation (and one I agree with) of the GitHub platform. The way to "work around it" is to set up guardrails around jobs (like triggering CircleCI) that require the job to be within the repo, not a forked repo. I planned to do this later in a follow-on PR for multiple jobs in the org, but that is an early next year thing, not a next week thing.
There was a problem hiding this comment.
Regarding next steps, I'm fine to make another pull request and move my changes there. In addition, I'm absolutely fine to just have this merged as is. I'll defer to others on the best approach for this.
There was a problem hiding this comment.
It isn't obvious, but I did do non-zero work on this PR.
Thanks for letting me know, @ryjones . I indeed didn't see this. So who would you see as the person that should be primarily attributed with this code (and how to properly attribute the other)?
My proposal would be to do it "as usual", i.e., ask @aidenfoxivey to do a PR from his/her repo and you propose your contributions during the PR: That way, both authors are recorded and we see CI run where possible (and trigger the rest). @aidenfoxivey, it would be nice if you'd locally execute the new test via CI (also checking that it actually runs :) before the PR. If you encounter problems in following the documented guidance on that, please let us know.
There was a problem hiding this comment.
@baentsch : @aidenfoxivey did most of the work, I did some of the work. I'm fine with having my attribution removed as Aiden would have made it there eventually.
I agree with having @aidenfoxivey create a new PR. As it's been approved twice, I think it should be merged as-is and allow Aiden to iterate in follow-up PRs.
baentsch
dismissed stale reviews from dstebila and Martyrshot
Test isn't testing; author attribution is wrong; please re-confirm approval if this is OK for you.
|
Moved my changes over to #1971 |
This adds a linking to cpp test.
This modifies the build action to build the test.