Nextcloud: Add phpExtraExtensions #109035
Conversation
turion
requested review from
talyz,
Ma27 and
lheckemann
and removed request for
talyz and
Ma27
turion
force-pushed
the
dev_nextcloud_php
branch
from
97fc764
to
6f4bb0a
Compare
| phpPackage = pkgs.php74.buildEnv { | ||
| extensions = { enabled, all }: | ||
| enabled | ||
| ++ (with all; [ apcu redis memcached imagick ]) # Necessary for vanilla nextcloud |
There was a problem hiding this comment.
Are apcu, redis, and memcached needed if caching.apcu, caching.redis, and caching.memcached are set to false?
There was a problem hiding this comment.
Hmm. Good question. Maybe not. With the new change, we could definitely make these extensions dependent on the options, and it's a good idea if it works.
It seems that the memcached option is actually never used for anything. Is that right?
There was a problem hiding this comment.
My impression is that in this commit, the extension setting was changed from optionally adding the three caching extensions to always adding them: ed20aae#diff-80ddb2d38f0f6e01125ccb8683381d7be20be1df3dce01e5c200dd74f5173623
So probably the right way forward now is to make them optional again, since this is easy now.
There was a problem hiding this comment.
No. See nixos/tests/nextcloud/with-mysql-and-memcached.nix.
Are apcu, redis, and memcached needed if caching.apcu, caching.redis, and caching.memcached are set to false?
I don't know. And I'd be especially cautious with apcu as this is something that's something where people often assume that it's "just available".
Is it really worth making this module even more complex to save the closure footprint of a single php extension though?
There was a problem hiding this comment.
In that case I would recommend removing those options if they do nothing.
There was a problem hiding this comment.
No. See nixos/tests/nextcloud/with-mysql-and-memcached.nix.
What do you mean? That test manually sets up memcached.
Is it really worth making this module even more complex to save the closure footprint of a single php extension though?
I don't know. Does the PHP extension for redis pull in redis-the-service/package as an additional dependency? Then it would be worth making it configurable. If it's just a few small PHP files, then I agree with @aanderse that the options should be removed, and the documentation adjusted that these extensions can always be assumed to be available.
There was a problem hiding this comment.
@Ma27 how shall we go forward?
| @@ -511,7 +533,6 @@ in { | |||
| pools.nextcloud = { | |||
| user = "nextcloud"; | |||
| group = "nextcloud"; | |||
| phpOptions = phpOptionsStr; | |||
There was a problem hiding this comment.
Have you confirmed whether this is actually fine?
There was a problem hiding this comment.
I took this commit from @aanderse . I don't know whether it is correct. It worked on my server, but I didn't tweak this setting.
There was a problem hiding this comment.
@Ma27 as you can see it is applied twice. I looked and confirmed duplicate config was generated.
| extensions = { enabled, all }: | ||
| enabled | ||
| ++ (with all; | ||
| [ imagick ] # Always enabled |
There was a problem hiding this comment.
This doesn't even evaluate since there's a missing ++.
It may be better to put ++ before a line, then it's also easier to comment a single line out during debugging.
| Nextcloud apps are installed statefully through the web interface. | ||
|
|
||
| Some apps may require extra PHP extensions to be installed. | ||
| This can be configured with the <link linkend="opt-services.nextcloud.phpExtraExtensions">nextcloud.phpExtraExtensions</link> setting. |
There was a problem hiding this comment.
That's confusing. The rest of the document uses the format I took. I don't know what the difference is.
There was a problem hiding this comment.
The text of the link is generated automatically by <xref />, while it needs to be specified with <link>…</link>. This is used throughout the manuals.
turion
force-pushed
the
dev_nextcloud_php
branch
from
e78d5ba
to
9005933
Compare
turion
force-pushed
the
dev_nextcloud_php
branch
from
9005933
to
8adb72e
Compare
turion
force-pushed
the
dev_nextcloud_php
branch
from
8adb72e
to
1d64cd9
Compare
turion
force-pushed
the
dev_nextcloud_php
branch
from
1d64cd9
to
0ff63a3
Compare
|
Rebased onto #110707 |
Motivation for this change
Fixes #107875
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)I tested on my local Nextcloud installation with
phpExtraExtensions = all: (with all; [ pdlib bz2 ] );. I could then run the https://apps.nextcloud.com/apps/facerecognition app which needs these extra extensions.Supersedes #108872.
CC @aanderse @talyz @lheckemann @etu @Ma27 @SuperSandro2000