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

Jump to bottom

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

Merged
merged 1 commit into from Mar 11, 2020
Merged

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

merged 1 commit into from Mar 11, 2020

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Feb 6, 2020 •
edited by aanderse

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.

@Ma27 Ma27 requested review from etu and aanderse February 6, 2020 19:15
@FRidh
Copy link
Member

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
Copy link
Member Author

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 February 17, 2020 11:24
@Ma27
Copy link
Member Author

Ma27 commented Feb 17, 2020

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

@aanderse
Copy link
Member

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

@etu
Copy link
Contributor

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
Closed
@Ma27
Copy link
Member Author

Ma27 commented Feb 27, 2020

Resolved the latest merge-conflict with master btw :)

@Ma27
php: add buildEnv function for additional config on the CLI SAPI
0bf5619 
Initially discussed in NixOS#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
Copy link
Member Author

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
View reviewed changes
Copy link
Contributor

@etu etu left a comment

Choose a reason for hiding this comment

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

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 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
@Ma27 Ma27 deleted the php-wrapper branch March 11, 2020 19:00
@aanderse
Copy link
Member

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

@etu etu mentioned this pull request Mar 11, 2020
Merged
9 tasks
This was referenced Mar 22, 2020
Closed
Closed
@etu etu mentioned this pull request Mar 31, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etu etu etu approved these changes

@aanderse aanderse Awaiting requested review from aanderse aanderse is a code owner

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Ma27 @FRidh @aanderse @etu