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

Add Nix flakes #800

Merged
merged 26 commits into from
Nov 23, 2022
Merged

Add Nix flakes #800

merged 26 commits into from
Nov 23, 2022

Conversation

guilhermehas
Copy link
Contributor

@guilhermehas guilhermehas commented May 15, 2022

@mortberg
Copy link
Collaborator

@MatthiasHu Can you please take a look at this :-)

@MatthiasHu
Copy link
Contributor

MatthiasHu commented Aug 10, 2022

Disclaimer: I am not familiar with nix flakes. :-)

I successfully tested the flake by running:

nix --extra-experimental-features "nix-command flakes" shell github:guilhermehas/cubical/7039feccf434861d16921522ebdf1bd2c3de5306

@guilhermehas Could you perhaps provide a minimal working example of how to import the library in another project via the flake file?

@guilhermehas
Copy link
Contributor Author

guilhermehas commented Aug 10, 2022

cat << EOF > test.agda                                                                                                                           
{-# OPTIONS --cubical #-}
open import Cubical.Foundations.Prelude
EOF

nix --extra-experimental-features "nix-command flakes" shell github:guilhermehas/cubical/52dfed7b703e6294b7acd11244cd838480839367#agdaWithCubical

agda -l cubical -i . test.agda

With these commands, I can use the cubical library of Agda.

Do you want to document it in some file such as README.md? And try to make everything in just one command?

@mortberg
Copy link
Collaborator

cat << EOF > test.agda                                                                                                                           
{-# OPTIONS --cubical #-}
open import Cubical.Foundations.Prelude
EOF

nix --extra-experimental-features "nix-command flakes" shell github:guilhermehas/cubical/52dfed7b703e6294b7acd11244cd838480839367#agdaWithCubical

agda -l cubical -i . test.agda

With these commands, I can use the cubical library of Agda.

Do you want to document it in some file such as README.md? And try to make everything in just one command?

Maybe this would fit better in INSTALL.md?

@MatthiasHu
Copy link
Contributor

Thank you, @guilhermehas !

I agree that INSTALL.md would be a good place to put the usage example. Perhaps in a short section called "Nix flakes instructions"?

@MatthiasHu
Copy link
Contributor

Also, a question about .github/workflows/ci-nix.yml: I assume that this CI job checks the whole library, as part of building the flake, which is already done by ci-ubuntu.yml. So does this mean that the library will be checked twice on every push to a pull request? Or perhaps only on every push to the master branch? I am not sure what the line on: [push] means.

@guilhermehas
Copy link
Contributor Author

I have put the instructions in INSTALL.md and also changed the CI to just work when deploying on the master branch.

@MatthiasHu
Copy link
Contributor

I assume that the purpose of the Nix CI job is to check whether the (automatically generated) flake.lock file (which determines in particular the precise Agda version) needs to be updated after some changes to the library. If I understand you right, @guilhermehas, you would like this job to run (at least) whenever a PR is merged into master.

Then this raises the question who is responsible for updating flake.lock when the Nix CI job fails.

@MatthiasHu
Copy link
Contributor

The procedure in the new section in INSTALL.md works for me.

@guilhermehas
Copy link
Contributor Author

after some changes to the library. If I understand you right, @guilhermehas, you would like this job to run (at least) whenever a PR is merged into master.

I think that we need just to update the lock file if the library is using a feature of a newer compiler.
But if there is any problem with nix flakes, I will come here to fix it.

@mortberg
Copy link
Collaborator

Is this ready to be merged now? @MatthiasHu

Is there be anything us developers will have to do in the future for example when releasing a new version of the library when there is a new version of Agda out? @guilhermehas

flake.nix Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
.github/workflows/ci-nix.yml Outdated Show resolved Hide resolved
.github/workflows/ci-nix.yml Outdated Show resolved Hide resolved
@guilhermehas
Copy link
Contributor Author

I tried to use this Agda flake, but it didn't work here. It looks like it is broken.

I put inputs.nixpkgs.url = "nixpkgs/nixos-22.05" , so now nixpkgs is pinned in one version.

@ncfavier
Copy link
Member

Right, vector-hashtables is broken in 22.05. I guess that's fine.

Maybe the most flexible option is to also provide an overlay, just like the agda flake does, so that people can use this with whichever nixpkgs and Agda they want.

@mortberg
Copy link
Collaborator

Is there be anything us developers will have to do in the future for example when releasing a new version of the library when there is a new version of Agda out?

You'll at least need to bump the version in flake.nix. The Agda version used here is tied to the version in nixpkgs, so you'll have to wait for it to land in nixpkgs and then do nix flake lock --update-input nixpkgs.

A probably better alternative would be to use the Agda flake so that we can pin Agda to whatever version we need.

Ok, I can already now promise that I will forget to do this... Is there some way of doing this that doesn't require any effort for us developers?

@ncfavier
Copy link
Member

There is an action for automatically updating the lockfile and sending PRs: https://github.com/DeterminateSystems/update-flake-lock

@ncfavier
Copy link
Member

Also, whatever steps need to be taken could just be documented in RELEASE.md :)

@mortberg
Copy link
Collaborator

Also, whatever steps need to be taken could just be documented in RELEASE.md :)

That would be great!

@guilhermehas
Copy link
Contributor Author

@ncfavier I created an overlay. Look if it is good.
I don't know if it is possible to change the Agda compiler using my overlay.

@guilhermehas
Copy link
Contributor Author

Also, whatever steps need to be taken could just be documented in RELEASE.md :)

I put the instructions in RELEASE.md.

.github/workflows/ci-nix.yml Show resolved Hide resolved
RELEASE.md Outdated Show resolved Hide resolved
flake.nix Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
flake.nix Show resolved Hide resolved
@felixwellen
Copy link
Collaborator

@guilhermehas : What is the status of this PR? Does it need more testing?

@felixwellen felixwellen self-assigned this Nov 20, 2022
@guilhermehas
Copy link
Contributor Author

@guilhermehas : What is the status of this PR? Does it need more testing?

For me, it is done and it does not need more testing.

Copy link
Contributor

@phijor phijor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested the Nix Flake, and it works as expected.

Apart from some minor issues, this looks ready to rebase, squash and merge.

default.nix Outdated Show resolved Hide resolved
@felixwellen
Copy link
Collaborator

@phijor : Thanks for joining!
@guilhermehas : Thanks for updating... Could you also rebase/merge master?

@guilhermehas
Copy link
Contributor Author

@phijor : Thanks for joining! @guilhermehas : Thanks for updating... Could you also rebase/merge master?

I merged the master to it.

@felixwellen
Copy link
Collaborator

I assume the second, slow, apparently not cached ci job added by this PR is intended - I don't mind it since it doesn't seem to slow down the old ci job.

@felixwellen
Copy link
Collaborator

@guilhermehas : Feel free to use the "Resolve conversation" button in the conversations above, if you think something is resolved.

@guilhermehas
Copy link
Contributor Author

@guilhermehas : Feel free to use the "Resolve conversation" button in the conversations above, if you think something is resolved.

I clicked to resolve the conversation.

@felixwellen
Copy link
Collaborator

Ok, then - looks like everything is resolved -> merging.

@felixwellen felixwellen merged commit 9118909 into agda:master Nov 23, 2022
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.

6 participants