-
Notifications
You must be signed in to change notification settings - Fork 644
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
Build on Nixpkgs/NixOS #4405
base: master
Are you sure you want to change the base?
Build on Nixpkgs/NixOS #4405
Conversation
I'm not a Nix expert, so sorry if this is misguided. How do we ensure that updates to Idris don't break the Nix build script? Can we reasonably run it from Travis, for instance? |
Makefile
Outdated
|
||
dist/setup-config: | ||
$(CABAL) configure $(CABALFLAGS) | ||
$(CB) configure $(CABALFLAGS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of renaming the CABAL and MAKE variables? These shorter versions are (slightly) more difficult for me to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those try to ensure the correct idris
is invoked via env IDRIS=$(IDRIS) ...
and I simply used those. I'm not particularly attached to any of those names though, what would you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not leave them as they were?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? Repeat explicitly like in env IDRIS=$(IDRIS) $(MAKE)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was commenting on renaming the CABAL variable to CB here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uses the original names now.
idris.cabal
Outdated
@@ -108,12 +108,12 @@ custom-setup | |||
|
|||
Flag FFI | |||
Description: Build support for libffi | |||
Default: False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These flags should stay as they are, because libffi can be difficult for Mac users and some people don't like the license of GMP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I agree, I'm sort of asking how should this be handled in the second bullet point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that their behavior should remain unchanged in the .cabal file at least. Nix users are a minority of Idris users, and I don't think that we should make it more difficult to build it the common way to make Nix packaging easier.
idris.nix
Outdated
@@ -0,0 +1,61 @@ | |||
{ mkDerivation, aeson, annotated-wl-pprint, ansi-terminal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this output from cabal2nix, or is it hand-written? If it's generated code, should it be in the repo, or should the repo contain instructions to generate it? I'm not sure what best practices are around that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the output from cabal2nix
with some hand-edits to make sure perl
, nodejs
and cabal
are available and to fix some problems with fsnotify
(IIRC) that forgets to add some frameworks to the propagated dependencies. I'm a newbie with Nix, so I don't have an answer for the best practices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shlevy , I seem to recall you being an experienced Nixer. Is this the right way to do things? (sorry if I remembered wrong)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we've quite reached a point yet where there are best practices here exactly, but my preferred approach would be:
a) Autogenerate the cabal-based Nix expression on-the-fly with callCabal2nix
b) Add any necessary modifications using overrideCabal
, overrideDerivation
, etc. after importing the base generated expression.
This won't be helpful to you now, but we're developing a set of libraries internally at Target that we're hoping to open source over the next few months that will make this workflow clean and easy and hopefully get it adopted more widely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to make these changes if/when the PR is in an otherwise-acceptable state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I just need to figure out how.
@@ -1,37 +1,45 @@ | |||
build: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes seem unrelated to the task of building Idris with nix. Could they be in a separate PR/commit? And do they maintain compatibility with BSD make?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't try to maintain compatibility with BSD make because Setup.hs already assumes GNU make (see mymake
definition).
I can extract that to a separate PR if desired, but I'd prefer to keep it in this PR as well, since I had problems with missed dependencies until I changed the Makefiles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. @markuspf , is this likely to cause a problem for BSD support?
test/basic010/run
Outdated
@@ -1,10 +1,4 @@ | |||
#!/usr/bin/env bash | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change this script? And likewise for reg039.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Third bullet point. I don't know yet why, it fails on Nixpkgs/Darwin when invoked via the TIMEOUT script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of that script is to ensure that there's not an infinite loop. Getting rid of it breaks these tests, so that shouldn't be done. Better to figure out why Nix isn't working with these.
A travis job building via nixpkgs would be the next logical step to ensure this doesn't bitrot. They already have infrastructure in place for Nix jobs. |
Good to hear that Travis supports Nix. It looks to me like there's already an Idris package in nixpkgs: https://github.com/idris-lang/Idris-dev/wiki/Idris-on-NixOS Why can't that be used here? |
(and thanks for this, by the way - I'm rushing through comments and have just realized that I'm likely coming across as a bit gruff) |
@david-christiansen Re the build expression in nixpkgs and reducing duplication:
|
@jacereda Have you tried the expression from nixpkgs, instead of this one? Presumably it won't require all the changes to the Cabal flags and Makefiles. |
@shlevy Thanks! |
@david-christiansen I have successfully installed idris via nixpkgs, but I haven't figured out how to use the expressions in nixpkgs for interactive development and things like running a single test after making a change. Maybe @shlevy can help with that? |
Oh, I found there's a |
idris.nix
Outdated
export DYLD_LIBRARY_PATH=`pwd`/dist/build | ||
export TARGET=`pwd` | ||
''; | ||
src = if lib.inNixShell then null else orig.src; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haskell packages have an env
attribute that is appropriate for nix-shell environments, have you tried that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand. Do you mean the src = ...
part is redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean rather than doing src = ...
here you can do something like let pkg = nixpkgs.pkgs.haskell.packages.${compiler}.callPackage ./idris.nix { }; in if inNixShell then pkg.env else pkg
in default.nix
. See the output of cabal2nix --shell
for an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll get rid of shell.nix and use that instead to avoid polluting the base directory.
Some problems with this PR:
Enabled FFI+GMP by default due to my low skills with cabal and Nix.Removes the TIMEOUT invocation for some scripts, I still need to figure out why those failed when invoked through it.Maybe someone has ideas on how to deal with those.