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

Add nginx perl modules #73198

Merged
merged 2 commits into from Nov 27, 2019
Merged

Add nginx perl modules #73198

merged 2 commits into from Nov 27, 2019

Conversation

tekeri
Copy link
Contributor

@tekeri tekeri commented Nov 11, 2019

Motivation for this change

Allows Nginx to have perl modules.

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 nix-review --run "nix-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.
Notify maintainers

cc @thoughtpolice @7c6f434c @fpletz @globin

@7c6f434c
Copy link
Member

Was Perl in the runtime closure before?

@tekeri
Copy link
Contributor Author

tekeri commented Nov 12, 2019

@7c6f434c No, it is new dependency.
Does this commit need some change? (e.g. add withPerlModule)

@7c6f434c
Copy link
Member

Then maybe create a way (like overriding perl argument to null or something) to build without perl? Nginx sounds like something useful on closure-size-conscious VMs and also small cross-compilation targets where Perl might not be reliable yet.

@tekeri tekeri force-pushed the add-nginx-perl-modules branch 2 times, most recently from 91b696d to 5b71f18 Compare November 14, 2019 05:30
@tekeri
Copy link
Contributor Author

tekeri commented Nov 14, 2019

Right, got it.
Then should perl be opt-in or opt-out?

@7c6f434c
Copy link
Member

I would prefer opt-in unless a specific feature/set of features implemented as Perl modules and planned for packaging sound useful enough.

@tekeri
Copy link
Contributor Author

tekeri commented Nov 15, 2019

I understand :)
Any other problem?

@7c6f434c
Copy link
Member

I am not sure if there is a cheap example of a perl module to add as a NixOS test for Nginx or not. (Or maybe it is a part of your larger Perl strategy and I will see it in a week…)

@tekeri
Copy link
Contributor Author

tekeri commented Nov 15, 2019

I don't have some perl module suitable for NixOS test since perl_module support is just needed for my private module (single file)...

FYI the following code can be used to run a perl module.

commonHttpConfig = ''
      ...
      perl_modules ${ nginx }/lib/perl5;
      perl_modules ${ makePerlPath [ HTTPDate ] };
      perl_modules ${ makePerlPath [ PHPSerialization ] };
      perl_modules ${ makePerlPath [ URI ] };
      perl_modules ${ ./perl_modules };
      perl_require my_private_module.pm;
      ...
'';
(...).locations."= /test".extraConfig = ''perl my_private_module::handler;'';

@7c6f434c
Copy link
Member

Maybe add perl = null at the toplevel then.

@tekeri
Copy link
Contributor Author

tekeri commented Nov 27, 2019

@7c6f434c Could you check new commit?

@7c6f434c 7c6f434c merged commit a5f2664 into NixOS:master Nov 27, 2019
@tekeri
Copy link
Contributor Author

tekeri commented Nov 28, 2019

@7c6f434c Thanks!

dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Dec 5, 2019
* nginx: enable perl_module if perl is given

* nginx: move `perl = null` to toplevel

(cherry picked from commit a5f2664)
@tekeri tekeri deleted the add-nginx-perl-modules branch January 14, 2020 07:53
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

2 participants