Add nix support

[rolfschr]

This PR add support for Nix as part of the NGI0 subprogram.

Usually, a Nix package would get packaged by first declaring (or packaging if needed) its dependencies. In the Python world, this is not so easy so several solutions have emerged (pypi2nix, poetry2nix and others). VulnerableCode has a couple of dependencies which are not yet packaged in nixpkgs and are fairly hard to integrate. I opted to use poetry2nix to get that part done. However, VulnerableCode is not a Poetry project so I included a (non-applied) patch to convert it (i.e. add the necessary files.) The patch is applied during the creation of VulnerableCode's Nix package. I have furthermore added a script that generates this patch file automatically, given setup.py and requirements.txt. The Nix package build will fail if requirements.txt have changed, too. Finally, there is a test script that creates a temporary database and runs the import.

Also, I'm happy to provide a Github Workflow to run the Nix build as well as some tests periodically.

[rolfschr]

This file is here for compatibility with the "pre-Flake" Nix world.

[sbs2001]

@rolfschr thanks !

I am not familiar with the Nix world, so I can't say much, however :

Skimming through the diff, I don't see the code setting up the env variable GH_TOKEN . Our readme currently doesn't mention it(and there's the updated one at #271 ) . VulnerableCode's some importers require the github token to make api calls to GH for gathering security data .

Also the DCO check is failing, could you sign off your commits ?

[rolfschr]

@sbs2001 Thanks for the quick feedback. I signed-off the commits. I am not sure how you would like me to set up the GH_TOKEN. Reading through the readme you mentioned I understand that this is something that the user has to setup in advance before actually running manage.py. So I don't see how the code should be changed. I can of course check for the existence of this env var in the test import script if that's what you meant.

[sbs2001]

@rolfschr yeah, I'm sorry for the GH_TOKEN thing , just realized I was asking to pre-set a token in a linux package ;)

I'm approving this, and we could merge this once @pombredanne has a look at this, he is unavailable atm, so this might take few days.

By the way, about the NGI link you mentioned, it requires some authorization. I'm guessing only the proposer has access to it, is that correct ?

[sbs2001]

@rolfschr I was wondering what's the advantage of using Nix packaging vs the already provided docker setup ? apart from not requiring to install docker itself.

[rolfschr]

@sbs2001 Well the advantage is you don't need Docker, obvious disadvantage: you need Nix ^^. Mainly, the advantage of using Nix in such a way is that the build will be reproducible. You did pin the Python requirements as well as the docker base image. I would guess it doesn't make such a big difference in our case.

[andir]

It would probably be wise to not use the unstable flake format for this but to use the "classic" packaging for Nix packages here. This format has not been formalized or finalized.

[rolfschr]

Thanks for reviewing! The NixOS Foundation decided to package Software related to the NGI Zero subprogram using the Flakes format. There is not obligation to include this upstream.

EDIT: improve wording

[andir]

This PR add support for Nix as part of the NGI0 subprogram.

That link you posted requires a login. Where can we obtain one?

[pombredanne]

@rolfschr Thank you ++ ... this looks great though I have a few questions:

1. we do not use poetry (and there is no plan to switch to using it for now) so I am not sure how to manage both pinned requirements and a poetry lock file at the same time (to keep them in sync and avoid confusion of multiple lockfiles at the root)

2. how could we test this packaging in the CI?

3. is there a way to put all scripts and packaging files in a subdirectory such as /etc/nix?

[rolfschr]

Hi @pombredanne,

thanks for your review.

1. we do not use poetry (and there is no plan to switch to using it for now) so I am not sure how to manage both pinned requirements and a poetry lock file at the same time (to keep them in sync and avoid confusion of multiple lockfiles at the root)

The PR contains a patch that "converts" the code base into a poetry project. This patch is not meant to be applied by you but is applied on the fly by Nix. It should just sit in the repo. The patch itself needs to be modified whenever the requirements.txt change. I have documented this here. I have a put some effort into this in order to make the process as friction-less as possible. The nix build will fail otherwise: As you pointed out, things have to be kept in sync.
I understand that this is not a straight forward way. I opted for this approach because too many of VulnerablceCode's dependencies are not in Nix and/or hard to convert.

2. how could we test this packaging in the CI?

I don't have experience with travis but probably I could just add another build job? Alternatively, I GitHub workflow would do.

3. is there a way to put all scripts and packaging files in a subdirectory such as `/etc/nix`?

Yes, I could do this.

Let me know what you think and I'll adapt the PR.

[pombredanne]

re:

Alternatively, I GitHub workflow would do.

This would be great.

Yes, I could do this.
Let me know what you think and I'll adapt the PR.

And that would be great too! thanks

[pombredanne]

@rolfschr do you mind adding back your DCO signoff?

[pombredanne]

@andir does everything looks good to you otherwise?

[rolfschr]

So I'm happy with the state as is. I moved files to to etc/nix, added a github workflow (runs once a month) and used mach-nix instead of poetry2nix. The latter is way better given Vulnerablecode does not intend to use poetry. You might want to merge/combine/... the GitHub workflow with your travis CI. The worklfow currenlty only imports alpine since the full import took 4hrs and was aborted by GitHub.

[pombredanne]

I would like to check whether I can replace the poetry2nix-based approach by a more elegant way. In any case, I'll let you know once I assume I don't change anything anymore. Just keep the PR open until then if you don't mind.

Sure thing!

The latter is way better given Vulnerablecode does not intend to use poetry.

thx ++

You might want to merge/combine/... the GitHub workflow with your travis CI. The worklfow currenlty only imports alpine since the full import took 4hrs and was aborted by GitHub.

FWIW, we are eventually setting up a server dedicated to continuously fetch and keep the data fresh and make it available via the API and dumps :)

[milibopp]

From a Nix perspective this looks pretty good to me 👍

I did a code review of the Nix files and also ran the flake checks locally as well as the ./test-import-using-nix.sh script used for CI for confirmation. Since the Nix checks run pytest against a Nix-provided Postgres database it makes sense to rely on that for functional integrity of the package.

I commented on a few minor nitpicks in the code, but that is mostly syntax and documentation.

(For reference, I am the Nix reviewer coming in from ngi-nix/ngi#73.)

[rolfschr]

From a Nix perspective this looks pretty good to me +1

I did a code review of the Nix files and also ran the flake checks locally as well as the ./test-import-using-nix.sh script used for CI for confirmation. Since the Nix checks run pytest against a Nix-provided Postgres database it makes sense to rely on that for functional integrity of the package.

I commented on a few minor nitpicks in the code, but that is mostly syntax and documentation.

(For reference, I am the Nix reviewer coming in from ngi-nix/ngi#73.)

@edibopp Thanks for your review! I worked in all your comments.

@pombredanne From my point of view, this PR can be merged now.

[rolfschr]

Meh, let me check the issue with the updated lxml package, first. This comes from #306

[rolfschr]

@pombredanne Soooo, finally, I think you could merge this. I had to update mach-nix's python dependency graph to include the latest lxml version (65b78f4) and have updated the documentation accordingly (3f732cd) in case this happens again.

[pombredanne]

@rolfschr Thanks!

[pombredanne]

LGTM! Thanks

[pombredanne]

All merged. Thank you ++ 🙇