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

[WIP] PHP: Make the default package more sane #82794

Closed
wants to merge 26 commits into from

Conversation

@etu
Copy link
Contributor

etu commented Mar 17, 2020

Motivation for this change

Our old PHP derivation was crazy big with a crazy amount of enabled extensions that also had a crazy amount of defaults that we pulled in, yet excluded some extensions that would have been nice to have by default.

This is a follow up on: #82348 to utilize #79377 more.

I made a chart of extensions in org-mode: http://ix.io/2evY

We had several insane defaults such as ldap, imap, pdo_odbc, gd but still missed things that are quite sane such as opcache.

The current PHP

The current closure size is:

245697960    /nix/store/4ga497cqvz04vcyw9lf7s74ymrc6r28g-php-7.4.3

And this contains things (among very others) like:

56161288    /nix/store/2zsgfrkp0hn878rnrp0azpmy76ljm2b0-linux-pam-1.3.1
56167840    /nix/store/y5166rf2lkxkf3991qm390vn92czgfgh-openldap-2.4.49
The new smaller PHP

Here we have a much smaller closure size:

141377496    /nix/store/kij6n7rxzj9y54fg8ik44hhii3s824yr-php-7.4.3
Breaking changes

I've still have to track down packages in nixpkgs that will break from this, some examples are probably nextcloud among others due to the missing database extensions.

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.
@etu etu changed the title PHP: Make the default package more saneSlim down default php PHP: Make the default package more sane Mar 17, 2020
@etu etu added the 8.has: clean-up label Mar 17, 2020
@etu etu changed the title PHP: Make the default package more sane [WIP] PHP: Make the default package more sane Mar 17, 2020
@jtojnar

This comment was marked as resolved.

];
</programlisting>

All native PHP extensions are available under <literal><![CDATA[phpPackages.exts.<name?>]]></literal>.

This comment has been minimized.

Copy link
@Ma27

Ma27 Mar 17, 2020

Member

Does anybody know if there's a way to link packages from NixOS docs? (IIRC those are only available via the package search (or probably the nixpkgs-docs), but I may be wrong).

This comment has been minimized.

Copy link
@jtojnar

jtojnar Mar 17, 2020

Contributor

There is not documentation for packages except for the package list and that is not linkable. Docbook has <package> element though.

This comment has been minimized.

Copy link
@etu

etu Mar 17, 2020

Author Contributor

I've switched to the <package> element and mentioned that the listed extensions that aren't loaded as default are available as loadable extensions. That makes it at least a bit discoverable to the end user reading the release log. See 42b24cf

, phpdbgSupport ? config.php.phpdbg or true

# Build a minimal php
, minimalBuild ? config.php.minimal or false

This comment has been minimized.

Copy link
@Ma27

Ma27 Mar 17, 2020

Member

Should we add a few more usage instructions in the nixpkgs docs? (e.g. doc/builders/packages or doc/language-frameworks?)

This comment has been minimized.

Copy link
@etu

etu Mar 17, 2020

Author Contributor

We don't seem to have any doc at all for PHP... I looked into the language-frameworks directory and found python there for instance... And yeah. Nobody has even started a php document...

This comment has been minimized.

Copy link
@jtojnar

jtojnar Mar 17, 2020

Contributor

Nixpkgs docs are supposed to be for building packages (package build support). I guess section for PHP build support like composer2nix might make sense but user docs is better suited for NixOS docs.

This comment has been minimized.

Copy link
@etu

etu Mar 17, 2020

Author Contributor

composer2nix isn't in nixpkgs at all (😞) and isn't documented either because of that.

It would be great to document the php.buildEnv behavior somewhere other than the code and in pull requests.

pkgs/development/interpreters/php/default.nix Outdated Show resolved Hide resolved
pdo_pgsql
]; })
];
</programlisting>

This comment has been minimized.

Copy link
@Ma27

Ma27 Mar 17, 2020

Member

Would it make sense to provide e.g. a phpMinimal or a phpExtended (with opinionated extensions) or even a phpFull?

This comment has been minimized.

Copy link
@etu

etu Mar 17, 2020

Author Contributor

I would say that the one I provide in this PR is a good enough base, and if you want to build a minimal one you can do that. And a full one would be to add all the extensions to get a full one. And that shouldn't require any compiling so it's fast to build.

And I find the one I provide by default in this PR small enough so we don't need a minimal one in my opinion.

I was thinking about it before though, that's why the minimal flag is there at all :)

But the minimal one has some issues to build it completely minimal. That is if you have a minimal build without --enable-mysqlnd you're not able to build phpPackages.exts.pdo_mysql because the php used that provides phpize needs to be built with --enable-mysqlnd to have mysqlnd.h available.

This problem could be solved to use a less minimal php to build the extensions and then use a minimal as base for the package used. But then you would end up with both of those in the closure so I'd rather avoid that... :-)

@etu
Copy link
Contributor Author

etu commented Mar 17, 2020

I have just completed the nixpkgs-review run again. All packages that aren't marked as broken does build. I will assume that all packages within phpPackages does work just as is.

The ones I'm not sure if they work during runtime are the following:

  • adminer
  • arcanist
  • drush
  • icingaweb2
  • kcachegrind
  • lsp-plugins
  • matomo-beta
  • nagios
  • nextcloud-news-updater
  • phoronix-test-suite
  • piwik
  • pulseeffects
  • qcachegrind
  • unit
  • wp-cli
  • yle-dl
@etu etu force-pushed the etu:slim-down-default-php branch from 52506ba to 6d36f6d Mar 17, 2020
@ofborg ofborg bot requested a review from Ma27 Mar 17, 2020
@etu etu force-pushed the etu:slim-down-default-php branch from 0d39249 to 903727a Mar 21, 2020
etu added 2 commits Mar 15, 2020
@etu etu force-pushed the etu:slim-down-default-php branch 3 times, most recently from 11f6350 to 428b55e Mar 22, 2020
@flokli
Copy link
Contributor

flokli commented Mar 22, 2020

Recap: I had a very nice (remote) session with @etu hacking on this today.

We need to do some more thinking about how to make this a bit less obstrusive (it currently breaks every NixOS module shipping software requiring some sort of database access).

This could be solved by explicitly listing the required modules. However, the current approach istn't that nice. We could improve it by making the "default" php package configurable through system configuration, or by adding (some few) extensions.

There were also some cleanups (php-unit and php-embed), and the initial addition of some docs about the PHP package tooling.

etu added 3 commits Mar 22, 2020
@etu etu force-pushed the etu:slim-down-default-php branch from a614f24 to 91b7860 Mar 22, 2020
talyz and others added 7 commits Mar 24, 2020
This moves yet more extensions from the base build to
phpPackages.ext. Some of the extensions are a bit quirky and need
patching for this to work, most notably mysqlnd.

Two new parameters are introduced for mkExtension - internalDeps and
postPhpize. internalDeps is used to specify which other internal
extensions the current extension depends on, in order to provide them
at build time. postPhpize is for when patches and quirks need to be
applied after running phpize.
The tests for many of the extensions run just fine, for some a small
portion fail. This runs the tests by default and disables the tests
extensions with any failing tests.
A slight rewrite of buildEnv which:

1. Makes buildEnv recursively add itself to its output, so that it can
   be accessed from any php derivation.

2. Orders the extension text strings according to their internalDeps
   attribute - dependencies have to be put before dependants in the
   php.ini or they will fail to load due to missing symbols.
Opcache significantly increases PHP's performance and is by default
enabled on Debian based distributions. Not having it enabled by
default results in a puzzling performance loss for anyone attempting
to migrate from Debian/Ubuntu to NixOS who is unaware of
this. Therefore, enable it by default.
…butes
@etu etu force-pushed the etu:slim-down-default-php branch from a1ac835 to cdbda19 Mar 25, 2020
@etu etu force-pushed the etu:slim-down-default-php branch from cdbda19 to b05c0af Mar 25, 2020
etu and others added 8 commits Mar 25, 2020
Older versions of PHP have a bug where header files are included in
the wrong order. This is only an issue when compiling the opcache
extension as a separate module, like we do.
Also, document why the patch is needed.
@etu etu mentioned this pull request Mar 27, 2020
6 of 10 tasks complete
@etu
Copy link
Contributor Author

etu commented Mar 27, 2020

This PR has grown, not because of bad reasons. And things have gone quite back and forth a few times and I don't like that. But I still want to do this. So I made #83514 where I've cherry-picked out the things we need and resolved all the conflicts and made the history cleaner and nicer.

@etu etu closed this Mar 27, 2020
@etu etu mentioned this pull request Mar 31, 2020
8 of 10 tasks complete
@etu etu deleted the etu:slim-down-default-php branch Apr 5, 2020
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

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