search

open-quantum-safe / liboqs

C library for prototyping and experimenting with quantum-resistant cryptography
https://openquantumsafe.org/
Other
1.91k stars 465 forks source link

Add Nix flake #1970

Open aidenfoxivey opened 3 weeks ago

aidenfoxivey commented 3 weeks ago

@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.

baentsch commented 3 weeks ago

Thanks for the proposal @aidenfoxivey ! Quick initial question (not knowing anything about nix): Does this have to reside in the top level project directory?

aidenfoxivey commented 3 weeks ago

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.

SWilson4 commented 2 weeks ago

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:

  1. The flake should be tested in CI somehow.
  2. There should be some sort of maintenance commitment to ensure the flake stays up to date—would you be willing to take this on?
aidenfoxivey commented 2 weeks ago

  1. The flake should be tested in CI somehow. Sounds good! I'll write up a little testing setup sometime this week.

  2. There should be some sort of maintenance commitment to ensure the flake stays up to date—would you be willing to take this on? Absolutely! I'm happy to be the designated flake maintainer.

aidenfoxivey commented 2 weeks ago

Could you also update the quickstart section of README.md to include instruction on how to use the flake?

I'll remove the instructions from the flake and put them in the quickstart along with other information.

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 take a look!

aidenfoxivey commented 2 weeks ago

I think it's maybe better to version the flake starting from 0.0.1, but alternative perspectives are certainly appreciated.

aidenfoxivey commented 2 weeks ago

Not sure what I did to boggle the code formatting lol. Maybe something in how I did the README was an issue?

aidenfoxivey commented 2 weeks ago

@baentsch I'll look into generating it by default

aidenfoxivey commented 2 weeks ago

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.

baentsch commented 2 weeks ago

LTGM! Thanks for the contribution. Please make sure the all the tests are passed before you merge.

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 ?

cothan commented 2 weeks ago

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.

praveksharma commented 2 weeks ago

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 nix develop. This would be easy to test via a weekly CI job -- on a ubuntu:latest image with just nix that must pass some cmake build tests.

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.

aidenfoxivey commented 2 weeks ago

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?

It's not strictly impossible, but it's only possible with some pretty unpleasant hacks or writing your own Nix code. I see the nix build workflow as ideally being the "canonical" configuration. (Like the one that the average person wants to build. Maybe common case is the right word.)

aidenfoxivey commented 2 weeks ago

Hmm. These are good points.

My feeling about PLATFORMS.md is that it's a tiny bit complex given that Nix works on

but we also don't want to require Nix to build anything. I view it less as a platform and more as a way to develop your own tooling AND build liboqs deterministically.

I'd prefer to mention it in the README just because it seems a shame to not mention it given that it can let you develop liboqs without having to worry about installing all the dependencies in your package manager.

baentsch commented 2 weeks ago

it seems a shame to not mention it given that it can let you develop liboqs without having to worry about installing all the dependencies in your package manager

Agreed. What about adding an entry to Nix to https://github.com/open-quantum-safe/liboqs/wiki/Platform-specific-notes-for-building-liboqs instead of the explicit README mention? But then again, I'm anyway not particularly happy about having this information in the Wiki and not the core repo (so that it makes it into releases). Upon re-reading the README I've got to say the "Quickstart" is not exactly "quick", surely not short, so this PR is making life easier for folks using Nix (a link to Nix in the documentation --wherever that winds up-- would by good in any case).

What about this thought @SWilson4 : Make the Linux/Mac quickstart section as short as the Windows one (basically just list apt get ... && cmake), pointing to a more complete documentation (current detail level) elsewhere? Add a one-liner for Nix there and more complete Nix options (as in this PR) at where the complete text for Linux/Mac winds up? This way, the Quickstart gets truly quick again (for people that know the tooling -- and the ones that don't have to read below).

praveksharma commented 2 weeks ago

"canonical" configuration. (Like the one that the average person wants to build. Maybe common case is the right word.)

I think this makes sense, I think this can also be expanded to include a set of common configurations in the future. That said, I think avoiding build options for now and including them in another PR after we've had a chance to think about suitable methods for testing them makes more sense.

Make the Linux/Mac quickstart section as short as the Windows one

I think this also makes sense but perhaps doing all of that here is out of the scope for this PR. Instead, if this PR chooses to just focus on deterministically setting up the dev environment then the "1. Install dependencies:" step under Linux and Mac can be edited to include a new "With Nix:" sub-sections. If that's okay I can create a new issue to track the streamlining of the Quick Start section.

How does that sound @aidenfoxivey @baentsch ?

aidenfoxivey commented 2 weeks ago

This sounds reasonable to me!

aidenfoxivey commented 1 week ago

@praveksharma Just to be clear, you want changes on the README, right?

praveksharma commented 1 week ago

@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.