buildLuaPackage: set makeFlags directly instead of through preBuild #29954

vyp · 2017-09-30T14:19:11Z

Motivation for this change

So that lua packages can override it without having to resort to setting makeFlagsArray in preBuild.

@orivej So this is untested (I only tested building a few select lua packages), but it seems like the // attrs (line 16) means that anything use buildLuaPackage will be able to override/set makeFlags just by setting it? Is this something like what you wanted? If not of course feel free to discard and close, or otherwise modify/fix.

cc @Wyvie from git blame.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

orivej · 2017-09-30T14:34:05Z

This is definitely better than the status quo, although $out needs to be $(out) for make to read the value from the environment.

vyp · 2017-09-30T14:37:30Z

Updated. I'm curious, could you explain the difference between $out and $(out) a little bit more?

vyp · 2017-09-30T14:47:18Z

Also cc @richardipsum.

orivej · 2017-09-30T14:52:01Z

$out is never expanded by Nix. ${out} would have been expanded if it were defined, but it never is. In scripts $out is expanded by Bash, but makeFlags are not processed as a script. They are passed literally to make, and thenmake initializes its variables from the environment, and evaluates $(var) into the value of its variable var.

vyp · 2017-09-30T15:15:00Z

@orivej Thanks for the comprehensive explanation, that explains why $out works in preBuild but not in makeFlags. :)

Also I forgot I should mention that my main concern is breaking any currently successful builds of lua packages (even though that seems unlikely). I would have tested building all lua packages if it weren't for my currently low disk space and low powered machine. I can probably get around to testing building of all the lua packages at some point if no one else does, when or if I get access to another slightly better machine.

orivej · 2017-09-30T15:40:30Z

I'll test this later, both without other changes and after simplifying existing lua-packages.

vyp · 2017-10-08T05:07:07Z

@orivej So I made a start on testing these. First I tested which packages on master actually do build without this change. To do this I came up with the following simple shell script (I'm not sure if there's a better way?):

build-lua-packages

#! /usr/bin/env bash

set -e
cd $(nix-instantiate --find-file nixpkgs)

nix-build -A luaPackages.luabitop
nix-build -A luaPackages.luacheck
# nix-build -A luaPackages.luaevent # disabled
nix-build -A luaPackages.luaexpat
nix-build -A luaPackages.luafilesystem
nix-build -A luaPackages.luaposix
nix-build -A luaPackages.lpty
nix-build -A luaPackages.lua-iconv
nix-build -A luaPackages.luasec
nix-build -A luaPackages.luasocket
nix-build -A luaPackages.luxio
# nix-build -A luaPackages.luazip # disabled
nix-build -A luaPackages.luazlib
nix-build -A luaPackages.luastdlib
nix-build -A luaPackages.lrexlib
nix-build -A luaPackages.luasqlite3
nix-build -A luaPackages.lpeg
nix-build -A luaPackages.cjson
nix-build -A luaPackages.mpack

nix-build -A luajitPackages.luabitop
nix-build -A luajitPackages.luacheck
nix-build -A luajitPackages.luaevent
# nix-build -A luajitPackages.luaexpat # master fails
nix-build -A luajitPackages.luafilesystem
nix-build -A luajitPackages.luaposix
nix-build -A luajitPackages.lpty
nix-build -A luajitPackages.lua-iconv
nix-build -A luajitPackages.luasec
nix-build -A luajitPackages.luasocket
nix-build -A luajitPackages.luxio
# nix-build -A luajitPackages.luazip # master fails
nix-build -A luajitPackages.luazlib
nix-build -A luajitPackages.luastdlib
nix-build -A luajitPackages.lrexlib
nix-build -A luajitPackages.luasqlite3
nix-build -A luajitPackages.lpeg
# nix-build -A luajitPackages.cjson # master fails
# nix-build -A luajitPackages.mpack # marked as broken

nix-build -A lua51Packages.luabitop
nix-build -A lua51Packages.luacheck
nix-build -A lua51Packages.luaevent
nix-build -A lua51Packages.luaexpat
nix-build -A lua51Packages.luafilesystem
nix-build -A lua51Packages.luaposix
nix-build -A lua51Packages.lpty
nix-build -A lua51Packages.lua-iconv
nix-build -A lua51Packages.luasec
nix-build -A lua51Packages.luasocket
nix-build -A lua51Packages.luxio
nix-build -A lua51Packages.luazip
nix-build -A lua51Packages.luazlib
nix-build -A lua51Packages.luastdlib
nix-build -A lua51Packages.lrexlib
nix-build -A lua51Packages.luasqlite3
nix-build -A lua51Packages.lpeg
nix-build -A lua51Packages.cjson
nix-build -A lua51Packages.mpack

The commented out lines represent those which do not build, either due to being disabled or marked as broken, or if they just fail to build. 3 luajit packages fail to build (which are not disabled or marked as broken), so they've been commented out.

So with this change, so far already luaPackages.luacheck fails to build with the following:

$ nix-build -A luaPackages.luacheck
these derivations will be built:
  /nix/store/knhxha3f43c3iny65da0a3z17lj1zy6h-lua5.2-luacheck0.20.0.drv
building path(s) ‘/nix/store/0vli0j701svgprgdy6gync8vx21qrf9c-lua5.2-luacheck0.20.0’
unpacking sources
unpacking source archive /nix/store/nxkk3wa4m8cqx6204jamcd1yh20agvwg-luacheck-0.20.0-src
source root is luacheck-0.20.0-src
patching sources
configuring
no configure script, doing nothing
building
build flags: SHELL=/nix/store/flb9ar1xdd13c606aa4my9miy3iv4vyk-bash-4.4-p12/bin/bash PREFIX=\$\(out\) LUA_LIBDIR=\$\(out\)/lib/lua/5.2 LUA_INC=-I/nix/store/2gx8l35r9b7arlmp9a1b8psbjlkfry1i-lua-5.2.3/include
make: *** No targets specified and no makefile found.  Stop.
builder for ‘/nix/store/knhxha3f43c3iny65da0a3z17lj1zy6h-lua5.2-luacheck0.20.0.drv’ failed with exit code
2
error: build of ‘/nix/store/knhxha3f43c3iny65da0a3z17lj1zy6h-lua5.2-luacheck0.20.0.drv’ failed

I have a feeling it could be the \$$out$ escaping going on with PREFIX and LUA_LIBDIR, but I'm not sure how that's happening or if that's expected or not.. 😕 Any ideas?

vyp · 2017-10-09T10:30:10Z

Ah I think I finally understand what's happening, it seems as though setting makeFlags means the buildPhase will run make, but because there's no Makefile (for luacheck), the builder fails and exits. Whereas without this change, the buildPhase simply doesn't do anything because it doesn't find any Makefile and then the installPhase (for luacheck) runs:

$ nix-build -A luaPackages.luacheck
these derivations will be built:
  /nix/store/dykqjx1fnlppjpzqfdg0rw2lbwglsmig-lua5.2-luachecc-0.20.0.drv
building path(s) ‘/nix/store/f9sy0fikjpm2wfd3hxzmbqnhzxd6w65k-lua5.2-luachecc-0.20.0’
unpacking sources
unpacking source archive /nix/store/nxkk3wa4m8cqx6204jamcd1yh20agvwg-luacheck-0.20.0-src
source root is luacheck-0.20.0-src
patching sources
configuring
no configure script, doing nothing
building
no Makefile, doing nothing
installing
Installing luacheck 0.20.0 into /nix/store/f9sy0fikjpm2wfd3hxzmbqnhzxd6w65k-lua5.2-luachecc-0.20.0

    Detected POSIX environment
    Writing luacheck executable to bin/luacheck
        Running chmod +x "bin/luacheck" >/dev/null
    Installing luacheck modules into /nix/store/f9sy0fikjpm2wfd3hxzmbqnhzxd6w65k-lua5.2-luachecc-0.20.0/src
        Running mkdir -p "/nix/store/f9sy0fikjpm2wfd3hxzmbqnhzxd6w65k-lua5.2-luachecc-0.20.0/src/luacheck" >/dev/null
        Running cp "src/luacheck/main.lua" "/nix/store/f9sy0fikjpm2wfd3hxzmbqnhzxd6w65k-lua5.2-luachecc-0.20.0/src/luacheck" >/dev/null
        Running cp "src/luacheck/init.lua" "/nix/store/f9sy0fikjpm2wfd3hxzmbqnhzxd6w65k-lua5.2-luachecc-0.20.0/src/luacheck" >/dev/null
        Running cp "src/luacheck/config.lua" "/nix/store/f9sy0fikjpm2wfd3hxzmbqnhzxd6w65k-lua5.2-luachecc-0.20.0/src/luacheck" >/dev/null

etc etc...

(I renamed the name to luachecc temporarily to force it to build because otherwise it just shows the path to the previously built luacheck.)

So what should be done now..? 😕

vyp · 2017-10-09T10:37:41Z

Okay it seems like simply setting dontBuild = true; does the trick for luacheck. @orivej Let me know if that's acceptable or not. I'll continue with testing the rest later. 👍

vyp · 2017-10-09T14:22:49Z

I'm not sure why the build for cjson fails for macOS, I don't use it.

So that lua packages can override it without having to resort to setting makeFlagsArray in preBuild.

marking it as broken

vyp · 2017-10-09T16:22:26Z

I think this is ready for merge now, I tested building all the rest and they work. Perhaps the macOS build failure for cjson happens without this change as well.

richardipsum · 2017-10-09T16:37:56Z

pkgs/top-level/lua-packages.nix

    src = fetchFromGitHub {
      owner = "ittner";
      repo = "lua-iconv";
-      rev = "e8d34024a6b185a759733915f116cc5588550261";
+      rev = name;


@vyp Curious why this is changing?

@vyp (Since the tag could be moved, which would then break the sha256)

Since the commit hash was tagged with that, it's simpler to just use the tag name instead. This is the usual case with most other nix package expressions, since the upstream authors usually follow the same pattern for naming new releases and tags, updating a package is usually a simple change to version and the src.sha256. Usually tags never move so it's fine. Does that explain?

If you want I could revert this but it would be irregular. In fact I'm surprised I didn't pick this up when reviewing #29949.

To be honest, my last commit here should have really been part of a separate PR I think, but oh well. :/

No need to revert, I'm the newcomer here 😄
I was just curious, thanks for explaining.

orivej · 2017-10-10T12:34:49Z

pkgs/top-level/lua-packages.nix

-    preConfigure = "substituteInPlace config --replace 'CC= gcc' '';"
+
+    preConfigure = ''
+      substituteInPlace config --replace 'CC= gcc' ''';


That's a bit confusing, they just meant --replace "CC= gcc" "".

Oh yeah that's simpler, good point. I was just mindlessly changing some formatting and wasn't thinking much. 🙈

orivej

@vyp Thank you very much! Sorry for the delay, I'm on a two week vacation and don't have much time for computer activities.

Perhaps the macOS build failure for cjson happens without this change as well.

hydra does not build cjson for darwin, so I'm going to assume that we are not breaking it.

I renamed the name to luachecc temporarily to force it to build because otherwise it just shows the path to the previously built luacheck.

This can also be accomplished with the --check option.

To do this I came up with the following simple shell script (I'm not sure if there's a better way?)

AFAIK classification of builds into those that were broker before the patch, broken after, or broken always, is not automated yet, and I hope to automate it.

luazlib

These involve: - Formatting and spacing. - Switching to using fetchFromGitHub where possible. - Adding missing meta attributes. - Correcting license values or adding missing license values. - Adding vyp as maintainer for unmaintained packages. None of these changes should affect the build result. A different revision is used for luasqlite3, however, the source code between these revisions is actually the same. (And the advantage is that the new revision is a tagged release.)

vyp · 2017-10-13T03:55:46Z

Sorry for the delay, I'm on a two week vacation and don't have much time for computer activities.

No problem at all, there's no rush, enjoy! 😄 🙌

hydra does not build cjson for darwin, so I'm going to assume that we are not breaking it.

Just to be clear, I didn't test on darwin myself, I was just referring to the travis build, but cool! 👍

This can also be accomplished with the --check option.

Oh sweet, thank you. This isn't on the man page for nix-build.

Thanks for taking the time to review during your vacation. 😊

richardipsum · 2017-10-16T19:51:10Z

@orivej Hi! I have more incoming changes to this file (additional packages). Should I base them on this PR? Or can I just adopt the conventions adopted in this file and submit based on master?

EDIT: I just read that you're on vacation currently, apologies for pinging you while on vacation.

vyp · 2017-10-16T23:27:50Z

@richardipsum Submitting a PR against master should be fine, go ahead. 👍 If it gets merged before this, and there's conflicts here, I can just rebase.

vyp force-pushed the fix/buildluapackage branch from ffe9711 to c5d444c Compare September 30, 2017 14:36

vyp force-pushed the fix/buildluapackage branch from c5d444c to e07a64b Compare October 6, 2017 03:20

vyp changed the title ~~buildLuaPackage: set makeFlags directly instead of through preBuild~~ [wip] buildLuaPackage: set makeFlags directly instead of through preBuild Oct 8, 2017

vyp added 4 commits October 10, 2017 02:51

buildLuaPackage: set makeFlags directly instead of through preBuild

df5691d

So that lua packages can override it without having to resort to setting makeFlagsArray in preBuild.

luajitPackages.mpack: disable with new isLuaJIT function insead of

b83c89b

marking it as broken

luajitPackages: disable luaexpat, luazip and cjson as their builds fail

152d294

luaPackages.luacheck: skip buildPhase

b584c46

vyp force-pushed the fix/buildluapackage branch 2 times, most recently from 2aa384d to 1faa29f Compare October 9, 2017 15:59

vyp changed the title ~~[wip] buildLuaPackage: set makeFlags directly instead of through preBuild~~ buildLuaPackage: set makeFlags directly instead of through preBuild Oct 9, 2017

richardipsum reviewed Oct 9, 2017

View reviewed changes

orivej reviewed Oct 10, 2017

View reviewed changes

orivej approved these changes Oct 10, 2017

View reviewed changes

vyp added 2 commits October 13, 2017 14:46

luaPackages: use consistent multiline strings for luafilsystem and

0946e30

luazlib

vyp force-pushed the fix/buildluapackage branch from 1faa29f to acc076d Compare October 13, 2017 03:49

orivej merged commit 839a479 into NixOS:master Oct 28, 2017

vyp deleted the fix/buildluapackage branch October 28, 2017 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buildLuaPackage: set makeFlags directly instead of through preBuild #29954

buildLuaPackage: set makeFlags directly instead of through preBuild #29954

vyp commented Sep 30, 2017 •

edited

orivej commented Sep 30, 2017

vyp commented Sep 30, 2017

vyp commented Sep 30, 2017

orivej commented Sep 30, 2017

vyp commented Sep 30, 2017

orivej commented Sep 30, 2017 •

edited

vyp commented Oct 8, 2017

vyp commented Oct 9, 2017

vyp commented Oct 9, 2017

vyp commented Oct 9, 2017

vyp commented Oct 9, 2017

richardipsum Oct 9, 2017

richardipsum Oct 9, 2017 •

edited

vyp Oct 9, 2017

vyp Oct 9, 2017

richardipsum Oct 9, 2017

orivej Oct 10, 2017

vyp Oct 13, 2017

orivej left a comment •

edited

vyp commented Oct 13, 2017

richardipsum commented Oct 16, 2017 •

edited

vyp commented Oct 16, 2017

buildLuaPackage: set makeFlags directly instead of through preBuild #29954

buildLuaPackage: set makeFlags directly instead of through preBuild #29954

Conversation

vyp commented Sep 30, 2017 • edited

Motivation for this change

Things done

orivej commented Sep 30, 2017

vyp commented Sep 30, 2017

vyp commented Sep 30, 2017

orivej commented Sep 30, 2017

vyp commented Sep 30, 2017

orivej commented Sep 30, 2017 • edited

vyp commented Oct 8, 2017

vyp commented Oct 9, 2017

vyp commented Oct 9, 2017

vyp commented Oct 9, 2017

vyp commented Oct 9, 2017

richardipsum Oct 9, 2017

Choose a reason for hiding this comment

richardipsum Oct 9, 2017 • edited

Choose a reason for hiding this comment

vyp Oct 9, 2017

Choose a reason for hiding this comment

vyp Oct 9, 2017

Choose a reason for hiding this comment

richardipsum Oct 9, 2017

Choose a reason for hiding this comment

orivej Oct 10, 2017

Choose a reason for hiding this comment

vyp Oct 13, 2017

Choose a reason for hiding this comment

orivej left a comment • edited

Choose a reason for hiding this comment

vyp commented Oct 13, 2017

richardipsum commented Oct 16, 2017 • edited

vyp commented Oct 16, 2017

vyp commented Sep 30, 2017 •

edited

orivej commented Sep 30, 2017 •

edited

richardipsum Oct 9, 2017 •

edited

orivej left a comment •

edited

richardipsum commented Oct 16, 2017 •

edited