agda: parameterize agda infrastructure by Agda executable name#452961
agda: parameterize agda infrastructure by Agda executable name#452961ncfavier merged 11 commits intoNixOS:masterfrom
Conversation
|
Sounds sensible. Is this used anywhere though? If not, maybe you can add a derivation to the tests that builds something with agda2hs? |
|
I intend to use it in: https://github.com/IntersectMBO/formal-ledger-specifications, where we use a custom agda backend. It is also possible to use it with
|
| ${lib.optionalString (ghc != null) ''--add-flags "--with-compiler=${ghc}/bin/ghc"''} \ | ||
| --add-flags "--library-file=${libraryFile}" | ||
| ln -s ${lib.getExe' Agda "agda-mode"} $out/bin/agda-mode |
There was a problem hiding this comment.
Is there a reason to remove this now? If not I would prefer to follow upstream's deprecation cycle. The executable currently prints a deprecation message when run.
There was a problem hiding this comment.
The reason is to be compatible with backends that do not have an agda-mode executable.
There was a problem hiding this comment.
This sounds like the kind of out-of-tree compatibility that would be hard to maintain in nixpkgs since we don't have any non-default backends here.
Adding a test derivation in nixpkgs as suggested by @turion could address this.
There was a problem hiding this comment.
I'd still prefer to conditionally link agda-mode if it exists.
Where would such derivation go? |
8156f06 to
f1efa8d
Compare
f1efa8d to
d56290e
Compare
philiptaron
left a comment
There was a problem hiding this comment.
I've got a bunch of completely non-blocking comments; my overall comment is that this use of meta.mainProgram as the configuration point seems odd and wrong to me.
Isn't the actual contract that whatever replaces agda needs to fulfill what it does, even if it uses a different name?
In other words, if agda2hs is used, the trivial derivation that makes it symlink to the $out/bin/agda name would also work for your purposes, as I understand it.
| @@ -203,7 +203,9 @@ in | |||
| adguardhome = runTest ./adguardhome.nix; | |||
| aesmd = runTestOn [ "x86_64-linux" ] ./aesmd.nix; | |||
| agate = runTest ./web-servers/agate.nix; | |||
| agda = runTest ./agda.nix; | |||
| agda = import ./agda { | |||
There was a problem hiding this comment.
Tangent about nested attrsets in this file
I see this is following the pattern of the rest of the imports in this file; in other files, I would have expected lib.recurseIntoAttrs to make the set of tests here built and eval'd by CI.
I don't really understand @roberth's change #191540 that introduced this stanza which deals with that...
nixpkgs/nixos/tests/all-tests.nix
Lines 113 to 125 in f991670
| assert "Hello World!" in machine.succeed( | ||
| "./HelloWorld" | ||
| ), "HelloWorld does not run properly" |
There was a problem hiding this comment.
Let's print what it did output:
| assert "Hello World!" in machine.succeed( | |
| "./HelloWorld" | |
| ), "HelloWorld does not run properly" | |
| text = machine.succeed("./HelloWorld") | |
| assert "Hello World!" in text, f"HelloWorld does not run properly: output was {text}" |
There was a problem hiding this comment.
I'm happy to add these suggestions. I was trying to be consistent with what was already in https://github.com/carlostome/nixpkgs/blob/d56290ec3cb5b490e5a6cb351e3e11b8df874789/nixos/tests/agda/base.nix
| pkgs.runCommand "trivial-backend" | ||
| { | ||
| version = "${pkgs.haskellPackages.Agda.version}"; | ||
| meta.mainProgram = "${mainProgram}"; |
There was a problem hiding this comment.
Isn't this:
| meta.mainProgram = "${mainProgram}"; | |
| meta = { inherit mainProgram; }; |
?
| name = "agda-trivial-backend"; | ||
| meta = { | ||
| maintainers = [ | ||
| # FIXME |
| hello-world = pkgs.writeText "hello-world" '' | ||
| {-# OPTIONS --guardedness #-} | ||
| open import IO | ||
| open import Level | ||
|
|
||
| main = run {0ℓ} (putStrLn "Hello World!") | ||
| ''; |
There was a problem hiding this comment.
I'd have landed this into the directory rather than using pkgs.writeText.
| cat << EOF > Main.hs | ||
| module Main where | ||
|
|
||
| import Agda.Main ( runAgda ) | ||
|
|
||
| main :: IO () | ||
| main = runAgda [] | ||
| EOF |
There was a problem hiding this comment.
I'd land this into the directory rather than use cat.
| pkgs.runCommand "trivial-backend" | ||
| { | ||
| version = "${pkgs.haskellPackages.Agda.version}"; | ||
| meta.mainProgram = "${mainProgram}"; | ||
| buildInputs = [ (pkgs.haskellPackages.ghcWithPackages (pkgs: [ pkgs.Agda ])) ]; | ||
| } |
There was a problem hiding this comment.
I'd use stdenvNoCC.mkDerivation here.
To answer this propertly we would have to define first "what (agda) does". For the change I'm proposing I have two cases in mind where one can be argued to fulfill what agda does and the other clearly does not but still can benefit from the agda nixpkgs infrastructure (
There is a small subtlety here. Agda backends "know" the name of their executable and use this for example with |
420027e to
71a7a61
Compare
|
I'm going to let @ncfavier and the Haskell / @NixOS/agda folks review/merge. I don't know enough about this domain to make the proper decision. |
Co-authored-by: Philip Taron <philip.taron@gmail.com>
- land `HelloWorld.agda` and `TrivialBackend.hs` in files - replace `runCommand` by `stdenvNoCC.mkDerivation` - refactor asserts
ef31bf8 to
55ac814
Compare
This PR addresses two issues:
build-support/agda/default.nixhas the name of the executable provided by theAgdadependency hardcoded everywhere. This prevents reusing the available infrastructure with other agda executables (e.g., provided by custom agda backends such asagda2hs).build-support/agda/default.nixexportsagda-modeexecutable. However, in version 2.8.0 this is deprecated. This PR removes this.Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.