-
Notifications
You must be signed in to change notification settings - Fork 517
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
Add Nix flake #1970
Add Nix flake #1970
Conversation
Thanks for the proposal @aidenfoxivey ! Quick initial question (not knowing anything about nix): Does this have to reside in the top level project directory? |
Hi @baentsch! In every project I've worked on, flake.nix and flake.lock do reside in the top level. That said, I was curious about the question, so I looked it up. According to this reference (https://nixos.wiki/wiki/Flakes), flake.nix does have to be in the top-level. My main goal with this is to have a declarative way to build liboqs to aid debugging and avoid having to use containers. |
Thanks for the submission @aidenfoxivey! I personally don't know anything about Nix, but I think two non–Nix-related issues would need to be resolved before this lands:
|
|
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.
The flake itself looks good to me, thank you @aidenfoxivey!
Could you also update the quickstart section of README.md to include instruction on how to use the flake?
Do you know if there's some way of parameterising the flake to spin up different dev envs, say with clang instead of gcc?
I'll remove the instructions from the flake and put them in the quickstart along with other information.
I'll take a look! |
I think it's maybe better to version the flake starting from |
Not sure what I did to boggle the code formatting lol. Maybe something in how I did the README was an issue? |
@baentsch I'll look into generating it by default |
5517c28
to
935fa2c
Compare
Turns out the version tag is completely unnecessary, since it can be referred to by its git hash anyways. I'll wait for additional comments and then clean up the git history once I've sorted out any requests. |
729315f
to
05edb28
Compare
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.
LTGM! Thanks for the contribution. Please make sure the all the tests are passed before you merge.
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.
This looks good, thank for you working on this @aidenfoxivey!
Hmm -- this statement made me curious: What tests are there (to be) passed? I didn't find any (just searching for changes to .github/workflows). Wouldn't that/having such be prudent, @cothan @praveksharma ? |
Hi @baentsch , I approve the PR while there are 6 tests (unrelated to the PR) are still running, since this PR doesn't add anything to Github flow, thus I expect them all to pass, and they did (I didn't aware at the time that each PR needs 2 approval). So I comment to make sure this PR will be merged in a green state. |
I agree that having testing for the flake would be desirable @baentsch but I'm also uncertain what is the best way to go about it. I believe the current build targets are limited in that they don't allow the user to pass CMake options via the nix CLI; is that correct @aidenfoxivey? Since the project doesn't plan on moving away from CMake, at this stage it might make more sense to focus on developer quality of life improvement via Alternatively, if there is some way of transparently exposing CMake options to nix that would also be suitable. Instead of a maintainer having to ensure compatibility with CONFIGURE.md that would be handled natively by nix. |
@praveksharma Just to be clear, you want changes on the README, right? |
Yes, listing the relevant nix alternatives to the apt commands under Quick Start > Linux and Mac > 1. Install dependencies is what I had in mind. |
As part of my year-end review of stalled things, can I ask what the idea is with this PR:
Which of these options would you like to pursue, @aidenfoxivey ? One, all, none, something else? |
Ah whoops! I've got to admit liboqs slipped by mind a little bit during the latter half of the term. I'm going to sort out the README section tonight. Thinking about testing, I think I wrote a solution but never pushed it. I'll look to wrap this up in the next few days. |
d480dac
to
de5beca
Compare
So I think there are a few things to think about:
Are all of these in scope for what tests should do? |
@baentsch Is this what you had in mind when it came to testing? |
It's a start - with item 3 being the most relevant imo. |
Okay, that functionality should be added now. Had to implement a small fix to prevent it from trying to rewrite a copy of the CMake file internally. |
Hi @aidenfoxivey, do you think you will be able to address the point above from security scanner? Is there anything else to be done before merging this? |
7d7b974
to
c74ef9d
Compare
I've just pinned them to the latest version I could find. I'm running it locally on my Mac to give it a check. |
Woohoo!
|
Thanks @aidenfoxivey! @baentsch you've got a review requesting changes; does this clear your concerns? If so then please clear that in your review and then we can merge. |
Dismissal requested. Still thinking testing would be appropriate, though.
Not really -- but this is not worth fussing over. |
c74ef9d
to
d6118d7
Compare
Should be moved into |
@baentsch I'm happy to make changes in this PR or a follow up to make this more robust. I apologize for the delays in the middle of this. (I'll be back at this after 5pm EST today to review comments and/or fix up the CI issues.) |
I don't believe we install Looks like you'll have to rebase to get CI to run. |
b63a870
to
cfc4e30
Compare
Seems the Nix tests are working - happy to squash and force push when it's all passed. |
Signed-off-by: Aiden Fox Ivey <[email protected]>
cfc4e30
to
3b8ba5f
Compare
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.
LGTM, thanks @aidenfoxivey!
@praveksharma @SWilson4 As I mentioned earlier, I wrote a little nix flake that should be able to run the environment without anything other than Nix installed on the machine - in addition to being declarative.
I'm happy to add or remove packages to the flake - I mostly followed the requirements for the QuickStart, but potentially missed something.