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

Lua generate nix packages from luarocks #54978

Merged
merged 5 commits into from Feb 4, 2019
Merged

Conversation

teto
Copy link
Member

@teto teto commented Jan 31, 2019

Motivation for this change

Extracted from #33903.

This adds an infrastructure to generate packages (from the whitelist maintainers/scripts/luarocks-packages.csv) from luarocks.org. It supports both the src.rock and rockspec format through the utility luarocks-nix (a fork of luarocks that adds a nix command). The infra is inspired by nixpkgs's haskell infra.

Running maintainers/scripts/update-luarocks-packages will update the content of pkgs/development/lua-modules/generated-packages.nix

The generated packages can't be added to lua environments yet because for ease of reviewing etc. I used a separate buildLuarocksPackage function but once this is merged, I will make all lua packages use the same buildLuaPackages function. For now, one can test for instance via
NIX_PATH="nixpkgs=/home/teto/nixpkgs2:$NIX_PATH" nix-shell -p lua5_3.pkgs.busted

I marked this Work In Progress because I still need to cleanup the wrap.sh script. I am looking for feedback on the rest though.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

done

# read returns a non-zero return code when it reaches EOF
# so disable the checks just for the 2 read
Copy link
Member

Choose a reason for hiding this comment

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

  1. Given you don't even use " you could as well do a normal variable assignment
  2. I would consider using || true with a comment instead of set +e — less chances of it getting its own life separate from the initial statements.

Copy link
Member Author

Choose a reason for hiding this comment

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

coudl you show some code please

Copy link
Member

Choose a reason for hiding this comment

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

HEADER="
/* ${GENERATED_NIXFILE} is an auto-generated file -- DO NOT EDIT!
Regenerate it with:
nixpkgs$ ${0} ${GENERATED_NIXFILE}

These packages are manually refined in lua-overrides.nix
*/
…
"

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. Done as you suggested. I removed the -e/+e that were helpful to diagnose weird read errors (zsh vs sh) that are not needed anymore.

Copy link
Member

Choose a reason for hiding this comment

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

And maybe kill the comment?

@7c6f434c
Copy link
Member

Are neovim/mpv changes from #54942 relevant here?

@teto
Copy link
Member Author

teto commented Jan 31, 2019

Are neovim/mpv changes from #54942 relevant here?

No, I will remove these commits on next push.

@teto teto changed the title [wip] Lua generate nix packages from luarocks Lua generate nix packages from luarocks Feb 3, 2019
@teto
Copy link
Member Author

teto commented Feb 3, 2019

I pushed an update that I mark as RFC. I reintroduced the wrapLua mechanism etc. After looking at it, I couldn't see much to do to reduce code duplication. It's like python's wrapping infrastructure.

Building is a bit verbose because of 2 kinds o messages:
Warning: The directory '' or its parent directory is not owned by the current user and the cache has been disabled. Please check the permissions and owner of that directory. If executing /nix/store/sgck3aj09nyhwni22bcp45cs62xnh84l-luarocks-2.4.4/bin/.luarocks-wrapped with sudo, you may want sudo's -H flag.

and:

Missing dependencies for busted 2.0.rc13-0:
   lua_cliargs == 3.0-1 (not installed)
   luafilesystem >= 1.5.0 (not installed)
   luasystem >= 0.2.0-0 (not installed)
   dkjson >= 2.1.0 (not installed)
   say >= 1.3-0 (not installed)
   luassert >= 1.7.8-0 (not installed)
   lua-term >= 0.1-1 (not installed)
   penlight >= 1.3.2-2 (not installed)
   mediator_lua >= 1.1.1-0 (not installed)

I believe these 2 can be worked around but I already spent much time on the lua branch and the current branch works so I would rather fix those later (also it's easier to work on it once it's merged).

What I was thinking that could help reduce friction while merging is to remove from the generated packages list those that are already present in nixpkgs. It should prevent failures while letting us some time to finalize the infrastructure (aka update some packages to use lua environments, alias buildLuaRocks to buildLuaPackage and write the doc). @7c6f434c what do you think ? (EDIT: I removed duplicated packages)

@teto
Copy link
Member Author

teto commented Feb 3, 2019

arf I need to add some with pkgs;. Will do on next push.

@7c6f434c
Copy link
Member

7c6f434c commented Feb 3, 2019

In general, maybe generate a small whitelisted package set of things we lack, then expand to handle more corner cases (nontrivial native dependencies etc.), then replace the current packages when everything is fine?

@teto
Copy link
Member Author

teto commented Feb 4, 2019

I agree with you, that's what I did in the commit "removed packages already available in nixpkgs". I left the overrides inplace since they mostly add buildInputs and should be harmless.
When this is considered fine for merging, feel free to squash the commits.

update script can now accept another csv file as input with -c
@teto
Copy link
Member Author

teto commented Feb 4, 2019

I added a -h flag while removing the "-p" one that allowed to specify a specifc package. It made the script more complex for no gain now that we can specify an alternative list.

@7c6f434c 7c6f434c merged commit 2ba8917 into NixOS:master Feb 4, 2019
@teto
Copy link
Member Author

teto commented Feb 4, 2019

thanks for the reviews ! Sorry I forgot about that old comment. I don't expect much (any?) breakage from this merge. I will prepare the next PR that will merge the different systems and should be the sensible one.

@Shados
Copy link
Member

Shados commented Feb 11, 2019

@teto you can remove the first warning by doing something like this before calling luarocks:

export HOME=$(pwd)
export USER=$(id -un)

On the second issue, I believe you'd need to create a Luarocks configuration file in the builder, with rocks_trees set to point to the rocks trees of the dependencies.

@vcunat
Copy link
Member

vcunat commented Feb 11, 2019

Related: #55553

@teto
Copy link
Member Author

teto commented Feb 11, 2019

@Shados patches welcome.

I don't think both problems are that hard to fix but it always takes time. I've tried your recommandation but it doesn't prevent the warning for me. As for the rocktrees, there is an example on how to do it on torch-distro.nix .

@Pablo1107
Copy link
Contributor

Should this be documented in nixpkgs manual?

@teto teto deleted the lua_automated branch June 9, 2020 08:48
@teto
Copy link
Member Author

teto commented Jun 9, 2020

@Pablo1107 there is #55302 that is ready to merge unless someone has suggestions.

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

Successfully merging this pull request may close these issues.

None yet

6 participants