luaiconv: init at 7 #29949

richardipsum · 2017-09-30T11:27:54Z

Motivation for this change

Hi!

I would like to package a git network auth package known as Gitano
lua-iconv is a dependency of Gitano's

I'll be opening separate PRs for each Gitano dependency, since that should make it easier to review.

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
Fits CONTRIBUTING.md.

This package has been tested on Debian through installation and test of Gitano itself.

Thanks!
Richard

orivej

Please submit Gitano and all of its dependencies in a single pull request (each package in a separate commit), this will make it easier to review and it will reassure that Gitano dependencies are packaged correctly.

orivej · 2017-09-30T11:39:40Z

pkgs/top-level/lua-packages.nix

+      sha256 = "0rd76966qlxfp8ypkyrbif76nxnm1acclqwfs45wz3972jsk654i";
+    };
+
+    buildInputs = [];


Delete this line.

If you need ~~this and the latter~~ preBuild to override buildLuaPackage defaults, just don't use buildLuaPackage.

Sorry I don't understand, I am really just following the convention of the rest of this file. I will ofcourse delete buildInputs but I don't understand why buildLuaPackage should be dropped 😕

Look at the definition of buildLuaPackage: https://github.com/NixOS/nixpkgs/blob/b5d11a76/pkgs/development/lua-modules/generic/default.nix
Basically it's mkDerivation with custom preBuild and buildInputs. I was not familiar with lua packaging and it appeared that you were attempting to circumvent its utility by overriding both of these attributes. Sorry.

Ahh I see okay, well I've left buildLuaPackage etc in tact, but have dropped the buildInputs line, moved the meta section to the bottom and added homepage 🐱

orivej · 2017-09-30T11:40:31Z

pkgs/top-level/lua-packages.nix

+      license = stdenv.lib.licenses.mit;
+      description = "Lua bindings for POSIX iconv";
+      maintainers = [ maintainers.richardipsum ];
+    };


Move meta to the end, use with stdenv.lib and with maintainers, add homepage.

orivej · 2017-09-30T11:41:13Z

pkgs/top-level/lua-packages.nix

+      makeFlagsArray=(
+        INSTALL_PATH="$out/lib/lua/${lua.luaversion}"
+      );
+    '';


Use makeFlags, you don't need preBuild for this.

I'm not familiar with that, but I'll look into it, thanks. 🐱

Everything else in this file seems to use preBuild 😕

Yeah, buildLuaPackage does not accommodate makeFlags… You may use this instead:
installFlags = [ "INSTALL_PATH=$(out)/lib/lua/${lua.luaversion}" ];

@richardipsum

makeFlags = [ "INSTALL_PATH=$out/lib/lua/${lua.luaversion}" ];

Edit: woops too slow

Looking at my build log I don't think this is required, currently, I see:

make[1]: Leaving directory '/tmp/nix-build-lua5.1-luaiconv-7.drv-0/lua-iconv-e8d34024a6b185a759733915f116cc5588550261-src' install -D -s iconv.so //nix/store/1khy7gdmz7fkim58ff05h7j2aflxb7hg-lua5.1-luaiconv-7/lib/lua/5.1/iconv.so post-installation fixup shrinking RPATHs of ELF executables and libraries in /nix/store/1khy7gdmz7fkim58ff05h7j2aflxb7hg-lua5.1-luaiconv-7 shrinking /nix/store/1khy7gdmz7fkim58ff05h7j2aflxb7hg-lua5.1-luaiconv-7/lib/lua/5.1/iconv.so

and when I modify the contents of INSTALL_PATH by appending a test string /TESTSTRING, then I see:

make[1]: Leaving directory '/tmp/nix-build-lua5.1-luaiconv-7.drv-0/lua-iconv-e8d34024a6b185a759733915f116cc5588550261-src' install -D -s iconv.so //nix/store/m9py1qwly24gf1mk5k7wqm3vdkq4dzab-lua5.1-luaiconv-7/lib/lua/5.1/TESTSTRING/iconv.so post-installation fixup shrinking RPATHs of ELF executables and libraries in /nix/store/m9py1qwly24gf1mk5k7wqm3vdkq4dzab-lua5.1-luaiconv-7 shrinking /nix/store/m9py1qwly24gf1mk5k7wqm3vdkq4dzab-lua5.1-luaiconv-7/lib/lua/5.1/TESTSTRING/iconv.so

This gives me some confidence that INSTALL_PATH is being picked up, other lua packages also set their install dir this way, as I say I really have just adopted the existing conventions used in this file.

@vyp This is a little deficiency of buildLuaPackage, it overrides makeFlags with its makeFlagsArray instead of extending them. We could fix it…

@richardipsum makeFlagsArray in preBuild is certainly correct, it's just not common outside of lua-packages when makeFlags is sufficient. Currently lua-packages have to resort to this (or, rather, set installFlags instead of makeFlags) because buildLuaPackage itself defines makeFlagsArray in its preBuild, but it does not seem to have a reason not to use makeFlags instead.

@orivej I see, noted, it's unclear to me what that means for the patches I have in flight. I would hazard a guess that were this to be fixed it should be done for all lua-packages to maintain consistency?

richardipsum · 2017-09-30T11:50:17Z

@orivej Thanks for reviewing so quickly! I will be happy to submit subsequent dependencies as part of one larger "Add Gitano" pull request if that will make this easier.

Thanks!

orivej · 2017-09-30T11:51:18Z

OK, but you can simply rename this PR and push additional commits.

richardipsum · 2017-09-30T11:57:47Z

@orivej My fear with opening one PR for the entire thing is that it will simply not be reviewed because it will be quite a large PR. Certainly for the first leaf deps (that are not core gitano components) I would prefer to keep them separate.

orivej · 2017-09-30T12:49:03Z

Upstream calls this package lua-iconv, not luaiconv. Would you rename it?

richardipsum · 2017-09-30T13:00:52Z

@orivej Good spot, renamed!

orivej · 2017-09-30T13:01:19Z

pkgs/top-level/lua-packages.nix

@@ -215,6 +215,31 @@ let
    '';
  };

+  lua-iconv = buildLuaPackage rec {
+    name = "luaiconv-${version}";


Update this too please.

woops, done 😸

richardipsum · 2017-09-30T13:05:35Z

We merged the version before I pushed the name change?!

orivej · 2017-09-30T13:09:39Z

@richardipsum No, I've merged right after your last commit.

richardipsum · 2017-09-30T13:12:07Z

@orivej Heh, you were too fast for github web ui, clearly, thanks for the review! 😸

orivej reviewed Sep 30, 2017

View reviewed changes

richardipsum force-pushed the init-luaiconv branch 2 times, most recently from f604b8b to 4266c2f Compare September 30, 2017 12:31

richardipsum force-pushed the init-luaiconv branch from 4266c2f to 25ecd13 Compare September 30, 2017 12:51

orivej reviewed Sep 30, 2017

View reviewed changes

lua-iconv: init at 7

cdd5176

richardipsum force-pushed the init-luaiconv branch from 25ecd13 to cdd5176 Compare September 30, 2017 13:03

orivej merged commit 6d8a32c into NixOS:master Sep 30, 2017

richardipsum deleted the init-luaiconv branch September 30, 2017 13:12

vyp mentioned this pull request Sep 30, 2017

buildLuaPackage: set makeFlags directly instead of through preBuild #29954

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

luaiconv: init at 7 #29949

luaiconv: init at 7 #29949

richardipsum commented Sep 30, 2017

orivej left a comment

orivej Sep 30, 2017

orivej Sep 30, 2017 •

edited

richardipsum Sep 30, 2017

richardipsum Sep 30, 2017

orivej Sep 30, 2017

richardipsum Sep 30, 2017

orivej Sep 30, 2017

richardipsum Sep 30, 2017

richardipsum Sep 30, 2017

orivej Sep 30, 2017

richardipsum Sep 30, 2017

richardipsum Sep 30, 2017

orivej Sep 30, 2017

vyp Sep 30, 2017 •

edited

richardipsum Sep 30, 2017

orivej Sep 30, 2017

orivej Sep 30, 2017

richardipsum Sep 30, 2017

richardipsum commented Sep 30, 2017

orivej commented Sep 30, 2017

richardipsum commented Sep 30, 2017

orivej commented Sep 30, 2017

richardipsum commented Sep 30, 2017

orivej Sep 30, 2017

richardipsum Sep 30, 2017

richardipsum commented Sep 30, 2017

orivej commented Sep 30, 2017

richardipsum commented Sep 30, 2017

luaiconv: init at 7 #29949

luaiconv: init at 7 #29949

Conversation

richardipsum commented Sep 30, 2017

Motivation for this change

Things done

orivej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orivej Sep 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyp Sep 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardipsum commented Sep 30, 2017

orivej commented Sep 30, 2017

richardipsum commented Sep 30, 2017

orivej commented Sep 30, 2017

richardipsum commented Sep 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardipsum commented Sep 30, 2017

orivej commented Sep 30, 2017

richardipsum commented Sep 30, 2017

orivej Sep 30, 2017 •

edited

vyp Sep 30, 2017 •

edited