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

[RFC] get lua support closer to python #33903

Closed
wants to merge 18 commits into from
Closed

[RFC] get lua support closer to python #33903

wants to merge 18 commits into from

Conversation

@teto
Copy link
Contributor

@teto teto commented Jan 15, 2018

Motivation for this change

My motivation is to be able to run neovim tests/plugins which rely on lua.
This PR can import a whitelist of packages (rockspec and src.rock) from luarocks automatically into nixpkgs.

Things done

Delegate lua packages handling to luarocks, similar to what python can do with pipy.

i have forked luarocks to generate a list of nix derivations from luarocks packages
https://github.com/teto/luarocks/tree/nix (needs cjson) and then run with luarocks nix [--server=SERVER] PKG_NAME

Right now, buildLuarocksPackage works with luarocks rocks (*.src.rock files = a zip archive with a rockspec describing the package + the source code) and rockspecs.

Possibie improvements

  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • 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.

Copy link
Member

@Mic92 Mic92 left a comment

Some comments while reading through. I have not yet run any tests.
Ideally your update script should also become part of nixpkgs.


#define build lua package function
buildLuaPackage = callPackage ../development/lua-modules/generic lua;
buildLuaPackage = with pkgs.lib; makeOverridable( callPackage ../development/interpreters/lua-5/mk-lua-package.nix {
Copy link
Member

@Mic92 Mic92 Jan 16, 2018

Choose a reason for hiding this comment

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

Maybe name the file also build-lua-package.nix for consistency.

@@ -0,0 +1,279 @@
/* ${GENERATED_NIXFILE} is an auto-generated file -- DO NOT EDIT! */
Copy link
Member

@Mic92 Mic92 Jan 16, 2018

Choose a reason for hiding this comment

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

Can you make you generator also to insert comment explaining how to regenerate this file in the header?

license=stdenv.lib.licenses.mit;
description="Event handling through channels"; }
;
src= fetchurl {
Copy link
Member

@Mic92 Mic92 Jan 16, 2018

Choose a reason for hiding this comment

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

Can you indent with two spaces in your generator and format the code like we do in nixpkgs in general?

Copy link
Contributor Author

@teto teto Jan 16, 2018

Choose a reason for hiding this comment

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

I will try to see what i can do in lua but I would rather run some nix prettifier. Any experience with https://github.com/Gabriel439/nixfmt or other advice ?

Copy link
Member

@Mic92 Mic92 Jan 16, 2018

Choose a reason for hiding this comment

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

nixfmt is probably not production ready. I am not aware of any other tool. Usual people write their tools in the same language as their target language. Then the likelihood is high that people, who are using the generator can also contribute back. Otherwise you can use the alignment feature of the printf function in bash.

src= fetchurl {

# url=http://luarocks.org/manifests/teto/coxpcall-scm-1.src.rock;
# sha256="0cz1m32kxi5zx6s69vxdldaafmzqj5wwr69i93abmlz15nx2bqpf";
Copy link
Member

@Mic92 Mic92 Jan 16, 2018

Choose a reason for hiding this comment

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

Does your generator also add comments?

Copy link
Contributor Author

@teto teto Jan 16, 2018

Choose a reason for hiding this comment

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

sorry xD


preBuild = ''
cd ..
ls
Copy link
Member

@Mic92 Mic92 Jan 16, 2018

Choose a reason for hiding this comment

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

Should be removed.

rm -rf $out/share/lua/${lua.luaversion}/cjson/tests
'';


Copy link
Member

@Mic92 Mic92 Jan 16, 2018

Choose a reason for hiding this comment

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

Spurious empty line.

. ${lua}/nix-support/setup-hook

if [ -L "$out/bin" ]; then
unlink "$out/bin"
Copy link
Member

@Mic92 Mic92 Jan 16, 2018

Choose a reason for hiding this comment

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

How is the semantic different from using rm here?

Copy link
Contributor Author

@teto teto Jan 16, 2018

Choose a reason for hiding this comment

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

copy pasted from

. According to man, the condition is entered only if $out/bin is a symlink. Not sure we need that.

Copy link
Member

@Mic92 Mic92 Jan 16, 2018

Choose a reason for hiding this comment

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

@FRidh when does the case triggers for python projects?

Copy link
Member

@FRidh FRidh Jan 18, 2018

Choose a reason for hiding this comment

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

Copy link
Member

@Mic92 Mic92 Jan 18, 2018

Choose a reason for hiding this comment

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

@teto I don't think we have that problem yet in Lua.

if [ -d "$dir" ]; then
echo "$dir is a folder"
find "$dir" -type f -perm -0100 -print0 | while read -d "" f; do
# Rewrite "#! .../env python" to "#! /nix/store/.../python".
Copy link
Member

@Mic92 Mic92 Jan 16, 2018

Choose a reason for hiding this comment

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

-            # Rewrite "#! .../env python" to "#! /nix/store/.../python". 
+            # Rewrite "#! .../env lua" to "#! /nix/store/.../lua". 

@@ -203,6 +203,9 @@ with pkgs;

fetchCrate = callPackage ../build-support/rust/fetchcrate.nix { };

gitRepoToName = callPackage ../build-support/fetchgit/gitrepotoname.nix { };

Copy link
Member

@Mic92 Mic92 Jan 16, 2018

Choose a reason for hiding this comment

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

Spurious empty line.

@teto
Copy link
Contributor Author

@teto teto commented Jan 16, 2018

Thanks for the review, I might have uploaded the PR a bit soon but I got a tad excited.
Regarding

Ideally your update script should also become part of nixpkgs.

Where should it go ? pkgs/build-support ? (EDIT: found it maintainers/scripts/ ) I believed the haskell generator lived outside of nixpkgs, its title is also "pkgs/development/haskell-modules"
/* hackage-packages.nix is an auto-generated file -- DO NOT EDIT! */

Right now the PR only supports src.rock format which doesn't exist for many packages. One long term solution could be to have nixos upload the missing src.rock to luarocks.org. nixos could fetch the rockspec and the source separately too.
Meanwhile I use a whitelist of packages I have tested (neovim dependancies). I believe it's enough for a first merge ?! the whitelist can be extended little by little. It is possible to fetch a list of all src.rock from luarocks and generate nix expressions from these but I am afraid packages might generate issues on install.

Also I forked luarocks in order to add the convert2nix command, should the fork live in nixpkgs too ? luarocks was supposed to support addons but I am not sure it is quite there yet. I will get some info.

@teto
Copy link
Contributor Author

@teto teto commented Jan 16, 2018

also when developing, I used to run luarocks --verbose to get more output. I suppose I should now enable this only when NIX_DEBUG > X . What the best way to do this ?

@Mic92
Copy link
Member

@Mic92 Mic92 commented Jan 16, 2018

Having a debugging mode and be silent, if no error happens is best practice.

@Mic92
Copy link
Member

@Mic92 Mic92 commented Jan 16, 2018

@teto if you don't want to add the generator to nixpkgs it is up to you. For go (go2nix), ruby (bundix), rust (carnix) and node.js (node2nix) we have packages in nixpkgs. For automatically generated package sets in nixpkgs we should always have documentation available on how to regenerate it.

@Mic92
Copy link
Member

@Mic92 Mic92 commented Jan 16, 2018

What parts of the rockspec-format does your generators support? https://github.com/luarocks/luarocks/wiki/Rockspec-format

@teto
Copy link
Contributor Author

@teto teto commented Jan 17, 2018

It supports the strict minimum aka homepage/name/version/url/checksum. I would like to convert licences too as I use a placeholder for now. Anyway I am preparing an update with your input so that package generation/etc can be tested without too much tinkering + some documentation. I'll try to use lua.penlight for pretty printing json nix

@teto teto changed the title get lua support closer to python [WIP] get lua support closer to python Jan 21, 2018
@teto
Copy link
Contributor Author

@teto teto commented Jan 27, 2018

Still a bit messy but at least it becomes usable by others (I think);
You can generate the packages via this command

nixpkgs$ maintainers/scripts/update-luarocks-packages.sh >  pkgs/top-level/lua-generated-packages.nix

@teto
Copy link
Contributor Author

@teto teto commented Jan 28, 2018

What I would like to do next is update the lua 5.1/5.3 interpreters to work as 5.2 does. Python copy/pastes the interpreters I believe but in this case I wonder if it's not easier to maintain to just override 5.2 with 5.1 source for instance ?

@teto
Copy link
Contributor Author

@teto teto commented Feb 2, 2018

I've updated luajit/5.1/5.3 to the new system. lua4 and lua 5.0 don't seem to be used anywhere so I propose to remove them (Lua 4.0 was released on 06 Nov 2000).
Neovim-dev pass all the functionaltests with lua5.1 (0.2.2 crashes during tests) !

According to nox-review (great tool !), I still have problems with "getLuaPath" so I should either reintroduce it or replace it (might require to support luarocks "external_dependencies" for "luaexpat") .

My main question at the moment is should I write all the generated packages in one huge "lua-generated-packages.nix" as haskell does and as I currently do or if I should put each package in a separate file and have one file using callPackage on each file (might consume too many inodes ?).

@Mic92
Copy link
Member

@Mic92 Mic92 commented Feb 20, 2018

@teto Sorry, I had a bit of a backlog. Regarding your file questions: If your package generation does not involve manual modifications of packages one file should be fine. For python most libraries are hand written, therefore multiple files are better to handle.

}


declare -A pkg_list=(
Copy link
Member

@Mic92 Mic92 Feb 20, 2018

Choose a reason for hiding this comment

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

Can you source a file containing pkg_list externally to separate program and data?

Copy link
Contributor Author

@teto teto Feb 27, 2018

Choose a reason for hiding this comment

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

done

@teto
Copy link
Contributor Author

@teto teto commented Feb 27, 2018

I've solved a couple problems on my way towards a successful nox-review wip aka luarocks2nix now generates some constraints like disabled = luaAtLeast "5.1" from the rockspec. Also I made the LUA_PATH more configurable since luajit interpreter installs files in some specific folders.

To make it more consistent with python (and easier to type), I would also like to rename lua5_1 to lua51, lua5_2 to lua52 etc. I wonder if that's ok.

@teto teto mentioned this pull request Jan 22, 2019
10 tasks
I should revert some majorVersion changes as I did in lua_step1.
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Jan 23, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/nixos-19-03-feature-freeze/1950/4

@danbst
Copy link
Contributor

@danbst danbst commented Jan 23, 2019

@teto (shameless plug) as you model this change from Python ecosystem, you probably understand that "overrides" are not very composable. I've been recently exploring the space of using overlays for defining package sets, and here is what I came up with:

  • define packages as an overlay, interpreter/packages.nix
self: super: {
    package1 = super.callPackage ./packages/package1.nix { };

    package2 = super.callPackage ./packages/package2.nix {
        dep1 = self.dep1.override {
            interpreter = self.interpreter;
        };
    };
}
  • define many interpreter versions in one overlay, interpreter/default.nix:
self: super:
let
  generic-interpreter = {
    stdenv, ...
    version, sha256,
    this
  }: ... ; 
in {
  interpreter1 = super.callPackage generic-interpreter { this = self.interpreter1; version = "v1"; sha256 = "..."; };
  interpreter2 = super.callPackage generic-interpreter { this = self.interpreter2; version = "v2"; sha256 = "..."; };
}
  • provide interpreters in all-packages.nix:
  inherit (import ../development/libraries/interpreter pkgs super)
    interpreter1
    interpreter2
  ;
  interpreter = interpreter1.override { this = interpreter; };
  interpreterPackages = recurseIntoAttrs interpreter.pkgs; # though it may be better to deprecate such at all
  • set passthru for interpreter as:
    passthru = {
      inherit version;

      pkgs = let
        scope = { interpreter = this; };
        newSelf = self // scope;
        newSuper = { callPackage = newScope (scope // this.pkgs); };
      in import ./packages.nix newSelf newSuper;

      withPackages = ... buildEnv ...;  
    };

And that's it!

You can use it Python like:

  • nix-shell -p 'interpreter.withPackages (ps: [ ps.package1 ])'
  • nix-shell -p interpreter.pkgs.package2
  • nix-shell -p interpreterPackages.package2

You can change the packageset via overlays:

self: super: {
  # this is completely custom package set, which won't interfer with nixpkgs
  interpreter-custom = self.interpreter.override { this = interpreter-custom; } // {
    pkgs = self.interpreter.pkgs // {
      package1 = self.interpreter.pkgs.package1.override ...;
      package3 = ...
    };
  };

  # this will do override in all nixpkgs
  interpreter1 = super.interpreter1 // {
    pkgs = super.interpreter1.pkgs // {
      package1 = super.interpreter1.pkgs.package1.override ...;
      package4 = ...
    };
  };
}

which is now composable, as many overlays can easily add/override packages.

Disclaimer: I've done this with postgresql, but it is not yet approved/reviewed/merged (https://github.com/NixOS/nixpkgs/pull/54319/files)

@teto
Copy link
Contributor Author

@teto teto commented Jan 24, 2019

@danbst thanks for the heads up/advice. The thread has become kinda messy but in fact while I started copying the python's approach, I ended up with the haskell way (well a mix of both). I've started this more than a year ago and would like to get the behavior merged (some things can be improved later certainly).
I've updated the first post with a roadmap on how I intend to split this PR. the target being 19.03.

@samueldr samueldr added this to the 19.03 milestone Jan 24, 2019
teto added a commit to teto/nixpkgs that referenced this issue Jan 30, 2019
First step towards more automation similar to the haskell backend.
Follow up of NixOS#33903
7c6f434c added a commit that referenced this issue Jan 30, 2019
* lua: add withPackages function

First step towards more automation similar to the haskell backend.
Follow up of #33903
@teto teto mentioned this pull request Jan 31, 2019
10 tasks
@lheckemann
Copy link
Member

@lheckemann lheckemann commented Feb 25, 2019

As far as I can tell this is superseded by smaller PRs that implement these changes piecewise? Can we close it?

@teto
Copy link
Contributor Author

@teto teto commented Feb 26, 2019

the most crucial part (luawithPackages) which was causing me pain when rebasing was merged so I close this (also because the page takes too long to load). Yet I like the idea of tracking some unmerged changes so I will create a new one.

@teto teto closed this Feb 26, 2019
@teto teto deleted the lua_clean branch Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment