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

nixos/httpd: fix lua paths #108511

Merged
merged 1 commit into from Feb 12, 2021
Merged

nixos/httpd: fix lua paths #108511

merged 1 commit into from Feb 12, 2021

Conversation

nagy
Copy link
Member

@nagy nagy commented Jan 5, 2021

Motivation for this change

My recent pull request #107649 contained an error. You could not use a direct lua package (e.g. the lua5_3 attribute) but had to use a derivation created with the withPackages helper that is in the lua attribute.

This changes makes it so that you can now also use an empty lua interpreter. In other words, this:

services.httpd.package = pkgs.apacheHttpd.override { luaSupport = true; };

gave an error, but with this change it works.

Notify Maintainers

@lovek323 @peti

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

diff LGTM

@nagy
Copy link
Member Author

nagy commented Jan 17, 2021

I have amended the commit with a fix that @aanderse wanted to have over at #107649 (comment)

@Mic92 Mic92 removed their request for review January 18, 2021 09:20
@aanderse
Copy link
Member

I wish I knew enough about lua to approve and merge this... @SuperSandro2000 are you comfortable to merge?

@SuperSandro2000
Copy link
Member

I wish I knew enough about lua to approve and merge this... @SuperSandro2000 are you comfortable to merge?

I know a bit lua but no in nixpkgs so I am not.

@aanderse
Copy link
Member

@teto @raboof @marsam @mjlbach or anyone who might have any relevant lua knowledge/experience to review this PR ... ping 🤷‍♂️

@nagy if we don't hear anything back in a few days please ping and we'll go ahead and merge this.

@Mic92 Mic92 self-requested a review February 10, 2021 10:22
@nagy
Copy link
Member Author

nagy commented Feb 10, 2021

I have just added a commit which includes the line

assert luaSupport -> lua5 != null;

in the apache package. This just makes the error message clearer when somebody forgets to set the lua5 attribute.

@SuperSandro2000
Copy link
Member

assert luaSupport -> lua5 != null;

This is not required and breaks overriding in certain cases. lua5 should never be null and give you a some other warning that it is marked as broken, insecure or does not work on your platform.

Account for the fact that, when creating a lua package without the
"withPackages" helper, we dont get an extra "lua" attribute in the
package.

Therefore we need to distinguish between the "withPackages" case and the
direct ( or "empty" ) lua package.

For example with this nixos config:

```nix
{
  services.httpd = {
      enable = true;
      package = pkgs.apacheHttpd.override {
        luaSupport = true;
        lua5 = pkgs.lua5_3.withPackages (ps: with ps; [ luafilesystem ] );
      };
    };
}
```

Here we say that we want to have apache to use a lua, packaged with the
`luafilesystem` module so that we can `require` that in scripts to
render http responses. There, the set that gets assigned to `lua5 ` does
not have a `luaversion` attribute, rather it has a `lua` attribute
wherein lies a `luaversion` attribute. If we dont package additional
modules, then we dont have that `lua` attribute in between and rather
directly have to use `luaversion` directly.
@nagy
Copy link
Member Author

nagy commented Feb 11, 2021

This is not required and breaks overriding in certain cases. lua5 should never be null and give you a some other warning that it is marked as broken, insecure or does not work on your platform.

Oh, you are right. I've removed that.

@marsam marsam merged commit ffedd32 into NixOS:master Feb 12, 2021
@nagy nagy deleted the apache-lua-path-fix branch February 12, 2021 12:12
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

5 participants