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: moving some package to the luarocks whitelist #55305

Merged
merged 5 commits into from Feb 12, 2019
Merged

Conversation

@teto
Copy link
Contributor

@teto teto commented Feb 6, 2019

Motivation for this change

Should be easier to maintain in the long run.

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.

@teto
Copy link
Contributor Author

@teto teto commented Feb 6, 2019

@teto
Copy link
Contributor Author

@teto teto commented Feb 6, 2019

In my tests 6 months ago with similar packages it worked out fine. I am just worried for mac users but luarocks should adapt its buildprocess accordingly.

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Feb 6, 2019

@GrahamcOfBorg build luaPackages.lgi

@teto
Copy link
Contributor Author

@teto teto commented Feb 7, 2019

regarding the darwin failure, the chmod is done by luarocks so I wonder what's wrong, maybe it gets a wrong shell sh: sw_vers: command not found.
Is the build flaky https://hydra.nixos.org/job/nixpkgs/gcc8/luaPackages.lgi.x86_64-darwin ?

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Feb 10, 2019

These are stop signs, not crosses — there are two successful builds and two manually cancelled ones

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Feb 10, 2019

@GrahamcOfBorg build luaPackages.basexx

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Feb 11, 2019

@GrahamcOfBorg build luaPackages.basexx luaPackages.mpack neovim

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Feb 11, 2019

chmod problem is still there…

@teto
Copy link
Contributor Author

@teto teto commented Feb 11, 2019

@GrahamcOfBorg build luaPackages.basexx luaPackages.mpack

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Feb 11, 2019

Apparently the patching of sw_vers doesn't happen

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Feb 11, 2019

Apparently we let some wrong chmod into the build; the chmod command comes from stdenv's unpackPhase (can be turned off via dontMakeSourcesWritable)

@alyssais
Copy link
Member

@alyssais alyssais commented Feb 11, 2019

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Feb 11, 2019

@alyssais Thanks for the explanation.

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Feb 11, 2019

@GrahamcOfBorg build luaPackages.basexx luaPackages.mpack luaPackages.lgi neovim

@teto
Copy link
Contributor Author

@teto teto commented Feb 12, 2019

@GrahamcOfBorg build luaPackages.basexx luaPackages.mpack luaPackages.lgi neovim

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Feb 12, 2019

@GrahamcOfBorg build luaPackages.basexx luaPackages.mpack luaPackages.lgi neovim

Aarch64 failure seems to be ENOSPC while building LLVM for Mesa for lgi

@teto
Copy link
Contributor Author

@teto teto commented Feb 12, 2019

I asked on nixos-dev for the machine to be fixed so hopefully there should be no space problem anymore. if darwin passes, I may move unzip to nativeBuildInputs. Is it going to be propagated ? in case someone wants to run luarocks unpack <src.rock> (is nativePropagatedBuildInputs a thing ?)

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Feb 12, 2019

The manual says you spell it wrong. propagatedNativeBuildInputs as per https://nixos.org/nixpkgs/manual/#ssec-stdenv-dependencies

@7c6f434c 7c6f434c merged commit 2a2cf5b into NixOS:master Feb 12, 2019
15 checks passed
@teto teto deleted the lua_whitelist branch Feb 13, 2019
vcunat
Copy link
Member

vcunat commented on c01fe37 Mar 4, 2019

Choose a reason for hiding this comment

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

This killed cqueues for other lua versions than 5.2. It worked fine for me with luajit so far. Unfortunately this doesn't seem possible to resolve via .override, as the overridden package throws an error already – so that's a general problem in this generator.

Apparently the package has three different *.rockspec for three different lua versions: https://luarocks.org/modules/daurnimator/cqueues I haven't looked into these luarock-generation changes – do you have any idea what to do about this? If not, reverting this particular commit should just work, until we do know.

BTW, you removed me from meta.maintainers, though I don't see that as a big deal ;-)

teto
Copy link
Contributor

teto commented on c01fe37 Mar 4, 2019

Choose a reason for hiding this comment

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

You are right, this is the first lua package I see with a rockspec per lua version.
I think this is one of a kind so maybe we could maintain overrides manually for each version.
Otherwise I could add a new field to maintainers/scripts/luarocks-packages.csv .

It's more like I didn't readd you rather than willfully removed you from maintainers xD As it gets automated and lua packages are rarely updated, I felt like it wasn't that important but as there is already an override, it's easy to readd the maintainers.

There is some doc on the lua changes at #55302 .
Feel free to revert the changes as I don't know when I can look into this. I will add it to #56398.

teto
Copy link
Contributor

teto commented on c01fe37 Mar 4, 2019

Choose a reason for hiding this comment

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

btw how did you write your override ? and what was the error ?

vcunat
Copy link
Member

vcunat commented on c01fe37 Mar 4, 2019

Choose a reason for hiding this comment

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

--- a/pkgs/development/lua-modules/overrides.nix
+++ b/pkgs/development/lua-modules/overrides.nix
@@ -6,6 +6,7 @@ with super;
   #### manual fixes for generated packages
   ##########################################3
   cqueues = super.cqueues.override({
+    disabled = false;
     nativeBuildInputs = [ pkgs.gnum4 ];
     buildInputs = [ pkgs.openssl ];
     extraConfig = with pkgs; ''

Error message, unchanged by the diff:

error: cqueues-20171014.52-0 not supported for interpreter /nix/store/ghcvhi7zsp2gby1ak8v9gwvgdkm15mk3-luajit-2.1.0-beta3

vcunat
Copy link
Member

vcunat commented on c01fe37 Mar 4, 2019

Choose a reason for hiding this comment

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

OK, reverted: 3e442fd, f06d8e0. And I needed to do a backport a36be6d; otherwise the it was behaving really weird on 19.03.

teto
Copy link
Contributor

teto commented on c01fe37 Mar 5, 2019

Choose a reason for hiding this comment

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

I don't pay too much attention to backports since I am always on unstable but a36be6d definitely needed a backport. I can ping you next time I see a commit needing backport.

I am thinking of changing the generator to generate cqueues_52, cqueues_53 etc derivations and then I will add a manual cqueue package that selects the good one depending on lua version.

vcunat
Copy link
Member

vcunat commented on c01fe37 Mar 6, 2019

Choose a reason for hiding this comment

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

That would probably work, though I'm not sure if it would be significantly better than the current non-generated state. (I don't really care, as long as it works well.)

teto
Copy link
Contributor

teto commented on c01fe37 Mar 7, 2019

Choose a reason for hiding this comment

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

yeah keeping it non generated might be the most sensible path as it's an unusual package. Nevertheless I investigated the issue and came up with some preliminary work here #57010

vcunat added a commit that referenced this issue Mar 4, 2019
This reverts commit c01fe37.
See the reverted commit on GitHub for discussion.  /cc PR #55305.
vcunat added a commit that referenced this issue Mar 4, 2019
This reverts commit c01fe37.
See the reverted commit on GitHub for discussion.  /cc PR #55305.

(cherry picked from commit 3e442fd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants