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

Support reproducible builds via Nix #1285

Merged
merged 60 commits into from Dec 3, 2015

Conversation

Projects
None yet
5 participants
@YPares
Collaborator

YPares commented Nov 4, 2015

This pull request implements the capability for stack to launch the build inside a nix-shell, in order to specify dependencies to libraries out of Hackage that exist in Nixpkgs.

Simply modify your stack.yaml so it contains for instance:

nix:
    enable: false
    packages: [glpk]
    #shell-file: shell.nix

Build this project with stack --nix build. It will open a Nix-shell that will first download the glpk dependency, and then launch the build. Of course, this requires nix to be installed on your system, and it requires your stack.yaml to depend on an LTS resolver that has been mirrored in the nix packages (see for instance in https://github.com/NixOS/nixpkgs/tree/master/pkgs/development/haskell-modules the configuration-lts-X.Y.nix files).
If you know nix, you can also specify a shell.nix that creates a derivation that will be used to set the build environment. You cannot specify packages and shell-file at the same time!
For now, this works much like the docker backend, by relauching the same stack executable inside a new environment. It requires GHC to be installed by Nix.

It's not possible to use stack ghci, because GHCi fails to find the extra libraries, but this is not specific to stack, as the command:
nix-shell --pure -p ghc extra-libs --command "stack ghci"
without enabling nix support in stack already fails.

Theoretically, activating both Nix and Docker could be possible (resulting in a nix-shell running inside a container), but that requires a Docker image providing Nix, of course.

YPares and others added some commits Oct 22, 2015

Remove ExecEnv abstraction. Allow docker + nix simultaneously.
Don't dispatch on the ExecEnv type. Using Docker and nix is no longer
mutually exclusive. We run commands in a docker container if configured,
and then fork a nix-shell inside if configured.
Using Strings instead of PackageName for nix packages
Using PackageNames forbid to use '.' in names, which is required when
you use attributes
hide
<*> pure []
<*> pure Nothing
<*> many (textOption (long "nix-shell-options" <>

This comment has been minimized.

@borsboom

borsboom Nov 26, 2015

Contributor

Use Options.Applicative.Args.argsOption instead of many . textOption, which will handle options within the value properly.

help "Additional options passed to nix-shell" <>
hide))
where
hide = if showOptions

This comment has been minimized.

@borsboom

borsboom Nov 26, 2015

Contributor

Use hideOpts here.

@@ -865,7 +880,10 @@ execCmd ExecOpts {..} go@GlobalOpts{..} =
-- Unlock before transferring control away, whether using docker or not:
(Just $ munlockFile lk)
(runStackTGlobal manager (lcConfig lc) go $
exec plainEnvSettings cmd args)
Nix.execWithOptionalShell

This comment has been minimized.

@borsboom

borsboom Nov 26, 2015

Contributor

You might be able to use reexecWithOptionalShell here. The Docker case has switched to always using reexecWithOptionalContainer (even with stack exec --plain) and you can probably do the same. After switching, that may open the opportunity to simplify Nix.rexecWithOptionalShell and Nix.execWithOptionalShell a bit.

@borsboom

This comment has been minimized.

Contributor

borsboom commented Nov 26, 2015

When I tried this with a resolver that hadn't been mirrored to nix yet, I got the following error:

error: attribute ‘lts-3_14’ missing, at (string):1:163
(use ‘--show-trace’ to show detailed location information)

After reading the docs I understood what this meant, but is it possible to make this more user-friendly? It would be nice if the error at least indicates why this would happen and perhaps suggests running nix-channel --update (if that makes sense). Perhaps it could even run the update automatically and retry, kind of like how Stack will download a snapshot automatically the first time it's encountered (not sure if Nix users would like automatic update though; I defer to your judgement about whether this is a good idea).

@borsboom

This comment has been minimized.

Contributor

borsboom commented Nov 26, 2015

Thanks for all the work on this, it looks really close to being ready to merge! Don't forget to add a changelog entry. It should go in the major changes section; this deserves lots of attention!

@YPares

This comment has been minimized.

Collaborator

YPares commented Nov 27, 2015

@borsboom I pushed a new commit addressing your comments.

Although, regarding this Nix error about the missing attribute, to provide a more informative error message I would need to ask nix about which attributes are available, which is not easy to automate as I pointed out in the manual.

The real fix would be to stop to depend on a GHC provided by Nix and use a GHC downloaded by Stack, but for now on Linux this results in failure to find libs in the nix store.

@borsboom

This comment has been minimized.

Contributor

borsboom commented Nov 27, 2015

Fair enough, I don't think the error message should hold up merging this, although it would be nice to improve at some point. I'm not surprised that trying to use a non-Nix GHC causes problems, so leaving that as-is is reasonable.

Do you think this may end up with similar problems as Docker in #911 and #1367? I'm beginning work on a fix for those two now, and most likely the same approach can be used for Nix as well.

Just filePath -> "from file: " <> (T.pack filePath)
Nothing -> "with nix packages: " <> (T.intercalate ", " pkgsInConfig))
e <- try (exec
(EnvSettings {esIncludeLocals = False

This comment has been minimized.

@borsboom

borsboom Nov 27, 2015

Contributor

You can use Stack.Config.Types.minimalEnvSettings instead.

fullArgs = concat [ -- ["--pure"],
map T.unpack (nixShellOptions (configNix config))
,nixopts
,["--command", intercalate " " (map (\a -> "'"++a++"'") (cmnd:args))

This comment has been minimized.

@borsboom

borsboom Nov 27, 2015

Contributor

Surrounding by ' isn't quite enough either; what if the value contain a ' itself? A reliable way to quote all characters, at least for bourne shell derivatives, is to surround the values in 's (like you did) and also replace all occurrences of ' within each value with '"'"'. Example code in Propellor's shellEscape.

Of course it would be much nicer if nix-shell gave you a way to do this without having to escape it at all, but I guess that's not possible.

@YPares

This comment has been minimized.

Collaborator

YPares commented Dec 1, 2015

@borsboom I corrected the escaping of the command line and re-merged the master.
Also, for coherence's sake, I made a slight adjustment: the section in the yaml is just called nix: now (no longer nix-shell:).
That's because the CL option was already --nix and because soon nix will adopt a git-like CL interface. I.e. the nix-shell tool will become just another subcommand of a big nix command.

@YPares

This comment has been minimized.

Member

YPares commented on 3554309 Dec 2, 2015

This fixed a bug that prevented a shell file to be used since Stack always added automatically ghc to the packages (hence a never empty list of packages).

borsboom added a commit that referenced this pull request Dec 3, 2015

Merge pull request #1285 from tweag/nix-merged
Support reproducible builds via Nix

@borsboom borsboom merged commit 931cb02 into commercialhaskell:master Dec 3, 2015

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@borsboom

This comment has been minimized.

Contributor

borsboom commented Dec 3, 2015

LGTM, thanks for this awesome new feature!

@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Dec 3, 2015

Awesome :D

I think this warrants a Major changes entry in the ChangeLog

@YPares

This comment has been minimized.

Collaborator

YPares commented Dec 3, 2015

@mgsloan @borsboom Thanks guys!

@YPares

This comment has been minimized.

Collaborator

YPares commented Dec 3, 2015

I will publish a blog post about this feature shortly.

@codygman

This comment has been minimized.

codygman commented Dec 13, 2015

@YPares looking forward to it, will you post it back here?

@borsboom

This comment has been minimized.

@mboes mboes deleted the tweag:nix-merged branch Dec 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment