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

php: add `buildEnv` function for additional config on the CLI SAPI #79377

Merged
merged 1 commit into from Mar 11, 2020

Conversation

@Ma27
Copy link
Member

@Ma27 Ma27 commented Feb 6, 2020

Motivation for this change

Initially discussed in #55460.

This patch adds a withPackages function to php that has the
following features:

  • php.buildEnv { extraConfig = /* ... */; } to specify custom
    php.ini args for the CLI.

  • php.buildEnv { exts = phpPackages: [phpPackages.apcu] } to
    create a PHP interpreter for the CLI with the apcu extension.


Just realized that it isn't that simple to implement a completely generic wrapper for the PHP ecosystem. I intentionally didn't add any docs, at first I'd like to know if the direction is correct :)

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.
@FRidh
Copy link
Member

@FRidh FRidh commented Feb 6, 2020

I think you should name it buildEnv instead of withPackages if it takes a set as argument, and let withPackages be a lambda, so to be consistent with other withPackages functions in Nixpkgs.

@Ma27 Ma27 force-pushed the Ma27:php-wrapper branch from 7b1811b to 6760345 Feb 11, 2020
@Ma27
Copy link
Member Author

@Ma27 Ma27 commented Feb 11, 2020

@FRidh you're absolutely right, fixed!
@aanderse regarding php-fpm support: I actually added it to the php-package now, it does not work with our module though as this adds yet another -c ${iniFile} :)

@FRidh FRidh changed the title php: add `withPackages` function for additional config on the CLI SAPI php: add `buildEnv` function for additional config on the CLI SAPI Feb 11, 2020
@Ma27 Ma27 marked this pull request as ready for review Feb 17, 2020
@Ma27
Copy link
Member Author

@Ma27 Ma27 commented Feb 17, 2020

@aanderse would you mind taking another look at this? :)

@aanderse
Copy link
Contributor

@aanderse aanderse commented Feb 23, 2020

@Ma27 I think this looks good 👍 I hope this has value to more than just me, though... What do you think @etu?

@Ma27 Ma27 force-pushed the Ma27:php-wrapper branch from 6760345 to 03a9223 Feb 24, 2020
@etu
Copy link
Contributor

@etu etu commented Feb 24, 2020

Hm, Interesting thing here. I've haven't had the time to test it yet. But it looks promising 🙂

@Ma27 Ma27 mentioned this pull request Feb 27, 2020
@Ma27 Ma27 force-pushed the Ma27:php-wrapper branch from 03a9223 to 9bbccb4 Feb 27, 2020
@Ma27
Copy link
Member Author

@Ma27 Ma27 commented Feb 27, 2020

Resolved the latest merge-conflict with master btw :)

Initially discussed in #55460.

This patch adds a `buildEnv` function to `php` that has the
following features:

* `php.buildEnv { extraConfig = /* ... */; }` to specify custom
  `php.ini` args for the CLI.

* `php.buildEnv { exts = phpPackages: [phpPackages.apcu] }` to
  create a PHP interpreter for the CLI with the `apcu` extension.
@Ma27 Ma27 force-pushed the Ma27:php-wrapper branch from 9bbccb4 to 0bf5619 Mar 9, 2020
@Ma27
Copy link
Member Author

@Ma27 Ma27 commented Mar 9, 2020

Resolved the merge-conflict. @etu any chance to get a review from you? :)

@etu
etu approved these changes Mar 10, 2020
Copy link
Contributor

@etu etu left a comment

I've just tested this locally and it totally works great. I think this is a good step for a more modular PHP setup.

And I mostly use PHP in CLI mode and do miss some modules sometimes that we have packaged but I haven't been bothered to get in... So big thanks for this PR :)

@Ma27
Copy link
Member Author

@Ma27 Ma27 commented Mar 11, 2020

@aanderse do you have anything to add? Otherwise I'd merge tonight :)

@Ma27 Ma27 merged commit e2d9520 into NixOS:master Mar 11, 2020
16 checks passed
16 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
php on aarch64-linux Success
Details
php on x86_64-darwin Success
Details
php on x86_64-linux Success
Details
@Ma27 Ma27 deleted the Ma27:php-wrapper branch Mar 11, 2020
@aanderse
Copy link
Contributor

@aanderse aanderse commented Mar 11, 2020

@Ma27 the only thing I have to add is a thank you for your work 😄

@etu etu mentioned this pull request Mar 11, 2020
7 of 9 tasks complete
@etu etu mentioned this pull request Mar 31, 2020
8 of 10 tasks complete
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

4 participants
You can’t perform that action at this time.