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 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

This comment has been minimized.

Copy link
Member

7c6f434c commented Nov 11, 2019

Was Perl in the runtime closure before?

@tekeri

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

7c6f434c commented Nov 13, 2019

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 tekeri:add-nginx-perl-modules branch 2 times, most recently from 91b696d to 5b71f18 Nov 14, 2019
@tekeri tekeri force-pushed the tekeri:add-nginx-perl-modules branch from 5b71f18 to 83cfa57 Nov 14, 2019
@tekeri

This comment has been minimized.

Copy link
Contributor Author

tekeri commented Nov 14, 2019

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

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Nov 14, 2019

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

@tekeri

This comment has been minimized.

Copy link
Contributor Author

tekeri commented Nov 15, 2019

I understand :)
Any other problem?

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Nov 15, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

7c6f434c commented Nov 16, 2019

Maybe add perl = null at the toplevel then.

@tekeri

This comment has been minimized.

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
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
nginx on aarch64-linux Success
Details
nginx on x86_64-linux Success
Details
@tekeri

This comment has been minimized.

Copy link
Contributor Author

tekeri commented Nov 28, 2019

@7c6f434c Thanks!

dtzWill added 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 tekeri:add-nginx-perl-modules branch Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.