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 flake support #199

Closed
wants to merge 4 commits into from
Closed

Add nix flake support #199

wants to merge 4 commits into from

Conversation

bodokat
Copy link

@bodokat bodokat commented Jul 4, 2023

Allows idris2-lsp to be installed using nix (see README)

@andorp
Copy link
Collaborator

andorp commented Jul 27, 2023

@ShinKage @michaelmesser Do you use nix? If yes, it would be nice to check this PR. If not, we can merge as it is.

README.md Outdated Show resolved Hide resolved
Co-authored-by: Luis Alberto Díaz Díaz <[email protected]>
Copy link
Member

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

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

I am looking forward to having a flake for the LSP. This is heading in the right direction, but it suffers from a couple of flaws, one of which makes it not build currently on Darwin systems.

First, I think we should be able to avoid building prelude, base, and contrib in this derivation because they are already built and added to the path of the idris2 executable we are building the LSP with.

Second, and more important, this uses buildIdris to build the idris2api which in its installLibrary derivation overrides buildPhase to be the empty string. The results in the default behavior of building the Idris 2 compiler when what we really want to do is build just the idris2api package using the existing Nix version of the Idris 2 compiler.

The first point is speculation on my part. The second causes a build failure on Darwin machines in addition to being a whole lot of extra work (building the compiler a whole time for no gain) even on systems for which it does succeed. I've got a partial proposed solution in the works, but for now I just wanted to comment with the problem.

@mattpolzin
Copy link
Member

mattpolzin commented Dec 22, 2023

Ok, fortunately the second point is easy to address; for example:

diff --git a/flake.nix b/flake.nix
index 207f608..366fb48 100644
--- a/flake.nix
+++ b/flake.nix
@@ -33,6 +33,11 @@
           echo "export idrisVersion : ((Nat,Nat,Nat), String); idrisVersion = $VERSION" >> src/IdrisPaths.idr
           echo 'export yprefix : String; yprefix="~/.idris2"' >> src/IdrisPaths.idr
           '';
+          # Perhaps counter-intuitively, we don't want to use the buildPhase provided by
+          # `buildIdris` becuase it will do a default build which will build Idris2, not
+          # the Idris2 API package. Fortunately, we can simply skip that buildPhase because
+          # `idris2 --install idris2-api` will build it as-needed in order to install it.
+          dontBuild = true;
         };

@mattpolzin
Copy link
Member

I did realize as I was switching back and forth between things that this PR predates the split of LSP-lib off into its own package. Definitely apologize this sat for so long. I hope that throwing the new package into the mix is no harder than the others built by this flake currently.

@andorp
Copy link
Collaborator

andorp commented Dec 25, 2023

I don't mind merging this PR, but I don't use nix. Are there any other nix users who can comment on these changes? If not, we can just merge this before the new year :)

@mattpolzin
Copy link
Member

mattpolzin commented Dec 25, 2023

@andorp i’d rather not merge this as is. I had a few requests above including one that fixes this derivation for use on Darwin machines. My most recent realization was that this also won’t work due to the introduction of the LSP-lib which isn’t yet accounted for in the changes here.

@andorp
Copy link
Collaborator

andorp commented Dec 25, 2023

Ok, just let me know when you think this is done.

@mattpolzin
Copy link
Member

I've opened a new PR that adds a flake here.

I made the fixes I proposed in this PR's comments, upstreamed the API package as suggested in a comment of the flake for this PR, and I built the relatively new LSP-lib dependency into the new flake.

@mattpolzin
Copy link
Member

This PR was the inspiration for #210 but it has now been superseded by that PR.

@mattpolzin mattpolzin closed this Jan 14, 2024
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.

4 participants