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
nixos/nextcloud: add phpPackage option #108872
Conversation
@GrahamcOfBorg test nextcloud |
I don't think we need an option to overwrite the php version. A list with extensions to enable would be much easier to use and less likely to break things. I am pretty sure Nextcloud does not support php80 yet or any much older version. |
Both options seem fine to me. |
This flexibility seems worthwhile to me. This option allows users to create a minimal |
I tested this successfully on my server. I enabled the |
and makes it harder to understand and way easier to do something breaking. Please compare the size of those two expressions:
Also the first is pinning php to 74 which is not ideal.
Nextcloud and minimal is a good joke. I don't think you can even create a more minimal php package set with the standard installation. |
I'm afraid it won't be that easy.
To match the existing conventions, we could also write it as:
The latter pattern also allows us to disable certain extensions, which may be what the users wants (although I didn't have such a use case myself yet). |
That's true. This is a real advantage of only selecting extensions instead of making the whole package. |
I agree with @SuperSandro2000 here: as maintainers we have to take care that a PHP we ship for Nextcloud works for the base module. It should be possible to use additional modules if needed and config can be written in the module itself. |
Then the best option is probably a pattern like |
This would be the easiest:
If we can't think about a usecase right now we should ignore it. If someone has a really good argument to add it we can always revisit it. |
You can drop the
instead. I do however agree that, if it's unnecessary to configure the version of PHP used, it would be better to only allow additional extensions to be specified.
That won't work, though; you'll get an undefined variable error when nix is trying to evaluate |
@aanderse What do you think is the best way forward? |
@turion - I think the best way forward is to add a I don't use the |
Ok, thanks for your work :) I'll make a new PR based on this. |
Some context: There was a commit 2 years ago, going like this:
This is why the package should not be configurable. |
Ok, makes sense, thanks! |
Motivation for this change
#107875
I don't use this module, so if @turion could provide testing it would be much appreciated.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)