[RFC 0169] Feature parameter names #169
Conversation
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/pre-rfc-standardization-of-feature-parameters/38104/74 |
rfcs/0168-feature-parameter-names.md
Outdated
| # Summary | ||
| [summary]: #summary | ||
|
|
||
| Nixpkgs usually exports compile-time options of the upstream build system |
There was a problem hiding this comment.
Maybe something more specific than «usually» should be said, as the majority of packages exposes no options (which happens sometimes because of upstream not having feature flags and sometimes because the maintainers do not see the benefits as sufficient to justify the effort).
There was a problem hiding this comment.
Rewrote to avoid any implications on how often upstream packages provide feature flags.
rfcs/0168-feature-parameter-names.md
Outdated
| * Further contributions that improve compliance with this RFC MAY involve | ||
| maintainers but SHOULD NOT be required to do so because the rules SHOULD be | ||
| clear. | ||
| * New packages MUST be compliant with the RFC (either no feature parameters or compliant with this RFC.) |
There was a problem hiding this comment.
Does the count starts from RFC passing, from Nixpkgs documentation updates being finalised, from the linter (with a working inhibition mechanism) being available, from the linter being deployed in CI?
There was a problem hiding this comment.
From passing of the RFC is implied. But in reality, people will keep violating the rules until there is automation that stops them from doing so. Promise checked, promise kept.
There was a problem hiding this comment.
The problem is that without «CI or go away», it's not like everyone will find out at once.
So there will be more useless nitpicking, and less uniformity (because gradual conversion away from established norms). And no time bound on this mess (as it starts with RFC passing and ends when CI is good enough to deploy).
There was a problem hiding this comment.
Well, If tomorrow I batch rename everything related to ALSA to with_alsa, new contributor day after will have no reason to invent alsaSupport if the only example he can find is with_alsa. But I agree, adding linter rule that would ban alsaSupport, withAlsa, useAlsa and other stuff I just renamed from together with the renaming would make it cleaner.
Not sure about performance, but on proof-of-concept level, it should be relatively simple treesitter query
There was a problem hiding this comment.
For various definitions of new. New contributor probably has already tried some kind of overriding before. Also, what happens to the release branch?
For CI, performance would be a consideration…
There was a problem hiding this comment.
It ended up better than I expected.
I wrote my tool inspired by nixpkgs-lint, and it takes ~30 seconds to process whole nixpkgs, around 15ms for single file.
rfcs/0168-feature-parameter-names.md
Outdated
| Boolean feature parameter MUST be either `with`-parameter or `enable`-parameter. | ||
| Non-boolean (e.g. string or integer) feature parameter MUST be an `conf`-parameter. | ||
|
|
||
| Feature paramter that incurs additional runtime dependency MUST be and `with`-parameter. |
There was a problem hiding this comment.
It looks like «and» is a remainder of some previous sentence structure? Of course «parmter» is a typo, but I hope it was «parameter».
rfcs/0168-feature-parameter-names.md
Outdated
| # being another one. | ||
| # | ||
| # I am not aware of anybody implementing X protocol themself, and not | ||
| # using one of these libraries. |
There was a problem hiding this comment.
sbcl.pkgs.clx does, but I am not aware of any Common Lisp package with an optional dependency on CLX. Rust has e.g. breadx.
rfcs/0168-feature-parameter-names.md
Outdated
| [Autoconf ENABLE](https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Package-Options.html) | ||
| [Autoconf WITH](https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Package-Options.html) |
There was a problem hiding this comment.
Is it intended to be the same link two times with two different labels?
rfcs/0168-feature-parameter-names.md
Outdated
| stdenv.mkDerivation { ... } | ||
| ``` | ||
|
|
||
| ## Feature parameter naming guidelines |
There was a problem hiding this comment.
There should be some example with a multi-word package being an optional dependency.
|
This is actually 0169 (based on the PR number) |
|
I guess PR title also needs an update. |
KAction
changed the title
edolstra
added
the
status: open for nominations
|
As proof-of-concept, implemented renaming logic on the Side effect of this approach is that it actually provides list of all known feature parameters. I'll probably move this approach as proposed, and current one as alternative solution. |
|
@infinisil Can I have your input? I think at 2024-01-09 nixpkgs architecture meeting you expressed interest in shepherding this one. |
rfcs/0169-feature-parameter-names.md
Outdated
| Developer interface for building environment with specific features enabled and | ||
| disabled is more complicated and requires more knowledge of implementation | ||
| details compared to already existing systems. | ||
|
|
||
| For example, Gentoo Linux has exhaustive list of [all known configuration | ||
| flags](https://www.gentoo.org/support/use-flags) and has means to | ||
| programmatically query what flags are available for particular package. Nixpkgs | ||
| has neither, and this RFC strives to rectify it. |
There was a problem hiding this comment.
You seem to describe the problem of .override attributes not being easily discoverable, but the RFC doesn't actually address that at all from what I understand.
If that's the actual problem you're trying to solve, the easiest solution might be to make those attributes more easily discoverable, e.g. by rendering attributes with their documentation on search.nixos.org. Or on the deep end, something like https://discourse.nixos.org/t/working-group-member-search-module-system-for-packages/26574
However, the actual problem you seem to want to solve with this RFC is described better in your comment here: NixOS/nixpkgs#234463 (comment). If that's indeed the case, the motivation section should be rewritten to talk about that.
There was a problem hiding this comment.
It actually does address, since now feature parameters are clearly namespaced from all other. To make it clearer,
I rewrote the motivation part as list of good things that we will get by implementing this RFC. Thoughts?
rfcs/0169-feature-parameter-names.md
Outdated
| 5. As side effect of the proposad migration plan is compiling exahustive list | ||
| of known feature parameters. It is not huge stretch to have CI check to | ||
| enforce that this list is current, bringing nixpkgs on par with Gentoo | ||
| [use-flags](https://www.gentoo.org/support/use-flags). |
There was a problem hiding this comment.
Doesn't Gentoo separately invest effort in consistently providing the popular USE flags for almost all the packages where they make any amount of sense?
There was a problem hiding this comment.
At some point did some manual comparison between Gentoo and Nixpkgs and learned that Nixpkgs exports no less of feature parameters than Gentoo, just in less uniform way. In other words, I expected Gentoo configuration to be more granular than Nixpkgs, but that wasn't the case.
I'll move the last sentence into the future work to not inadvertently imply that RFC demands Nixpkgs to have as much feature parameters as Gentoo has use flags.
There was a problem hiding this comment.
My expectation was that Nixpkgs has a comparable number of feature parameters, but the corresponding feature parameters are provided by fewer packages, isn't it so?
There was a problem hiding this comment.
Probably. Many packages do something like
buildInputs = lib.optionals stdenv.isLinux [ foo ];
without bothering to introduce with_foo flag.
|
Inviting reviewers of #234463 into the RFC discussion. Feedback is welcome. Finding more people to get RFC
is also welcome. I think on my side, I did proof-of-concept both on how to lint new packages and how to do renaming in existing. Anything I miss? |
|
My current opinion is: motivation mismatches the measures proposed; switching the naming word-handling convention is costly and the changes elsewhere have deprived it of the main benefit in terms of linting; the linting being mandated but not the trigger condition for the more onerous requirements timing is bad. Unification of option naming where only the names already used in practice are eligible — via should-merge stipulation for zero-rebuild PRs in that direction — would not bring practical benefits either but at least would be mostly harmless. I won't actively support even the latter reasonable version of unification, so I guess I am too biased to be a shepherd. I hope though, that I manage to keep the local feedback useful either to reject the substance and not silly technicalities, or to implement it with less unnecessary cost, useful or not. (Costs inherent to the claimed benefits are a trade-off question, we will never get agreement there, only a truce) |
rfcs/0169-feature-parameter-names.md
Outdated
| to the package. With naming mandated by this RFC, distinction is clear to | ||
| cater to automatic queries. | ||
|
|
||
| 5. Side effect of the proposdd migration plan is compiling of almost |
There was a problem hiding this comment.
| 5. Side effect of the proposdd migration plan is compiling of almost | |
| 5. Side effect of the proposed migration plan is compiling of almost |
This is not the intent/motivation of the MUST requirement, just an observation of a potential side-effect. I proposed the MUST requirement for |
However, the currently proposed rules for what is the real dependency parameter to mention are complicated enough that no automation is plausible, so we can just write should-accept for unifying renamings and keep camel case — given that for most cases it is clear what is the proper camel case. |
|
@wolfgangwalther shepherding is probably the best way you can help this RFC get forwards, so we'd like to nominate you as a shepherd -- let us know if you're alright with that :) |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/rfcsc-meeting-2024-04-16/43512/1 |
There was a problem hiding this comment.
First of all, I'd like to thank @KAction for putting all of this together. From the Pre-RFC to this, it's clear that a lot of thought and consideration was put into this.
There are some minor details I have some opinions on but the large picture gets an ACK from me.
| # Assertions are optional and might be infeasible when package has huge amount | ||
| # of feature flags, but in general improve user experience. | ||
| assert enable_ssl -> with_openssl || with_wolfssl; |
There was a problem hiding this comment.
Mutually exclusive features are a much better use-case for assertions. The information that enable_ssl requires one of the other flags to be set is already encoded in the default.
| # Assertions are optional and might be infeasible when package has huge amount | |
| # of feature flags, but in general improve user experience. | |
| assert enable_ssl -> with_openssl || with_wolfssl; | |
| # Assertions are optional and might be infeasible when package has huge amount | |
| # of feature flags, but in general improve user experience. | |
| assert !(with_openssl && with_wolfssl) # You can only have one! |
| no static code analyzis is perfect, it shall have support for inhibiting | ||
| warnings in unsual cases. | ||
|
|
||
| All feature parameters SHOULD be exported in `passthru.features` set. See example above. |
There was a problem hiding this comment.
I don't think this is a reasonable policy. If I did that for ffmpeg, the vertical size of the file would increase by 25% just due to this boilerplate.
This should be limited to the parameter names and then automated, ideally by default. The possibility to automate is one of the reasons we have the naming convention.
I propose this should be a list of parameter names be populated from the "outside" by taking package function's functionArgs's attrNames.
|
|
||
| * In existing packages, maintainers MAY start to expose feature parameters and MAY cease exposing them. | ||
| * Further contributions MAY change their package to be compliant with the RFC | ||
| * Further contributions MUST NOT rename non-compliant feature parameters to another non-compliant name. |
There was a problem hiding this comment.
I get the intention but requiring the change of all the feature parameters just because one needs a minor adjustment feels a little too strict to me. The intention is to make their amount gradually decline, not make it impossible to touch them.
This should be reduced to a SHOULD or removed entirely.
| * Further contributions MUST NOT rename non-compliant feature parameters to another non-compliant name. | |
| * Further contributions SHOULD NOT rename non-compliant feature parameters to another non-compliant name. Parameter naming SHOULD be made compliant first. |
| ### Technical process | ||
|
|
||
| Renaming the parameters is handled automatically by the `makeOverrideable` | ||
| function, as implemented in [https://github.com/NixOS/nixpkgs/pull/284107](#284107). |
There was a problem hiding this comment.
This link does not work.
| function, as implemented in [https://github.com/NixOS/nixpkgs/pull/284107](#284107). | |
| function, as implemented in https://github.com/NixOS/nixpkgs/pull/284107. |
| 2. Adding renaming rule into | ||
| [https://github.com/KAction/nixpkgs/blob/contrib/0/rfc169-proof/out/lib/rfc0169.json](makeOverrideable) | ||
| configuration, if it is not there already. |
There was a problem hiding this comment.
Also please explain what exactly editing that file is supposed to do, it is not clear.
| 1. Nix language provides way to [introspect function argument names](https://nixos.org/manual/nix/stable/language/builtins.html#builtins-functionArgs), | ||
| but no way to learn their default values. Learning default values is fundamentally impossible, since default values of one parameter might depend | ||
| on the value of another parameter, so default values can't be evaluated until function is called. | ||
|
|
||
| 2. Overrides become much more verbose. Simple and ubiquitous |
There was a problem hiding this comment.
Long-term, I'd prefer to move the flags and their default values out of the functions arguments all together.
Functions arguments are not a good place to define APIs and are already overloaded as is (is lib part of the drv's API? I don't think so).
This would entirely avoid the first problem as the args and defaults would be visible from the outside as a plain attrset.
As for the overrides, I don't think having many override functions for specific purposes such as overrideFeatures would be such a bad idea as they would represent a separations of concerns.
Though we're not there yet. See i.e. NixOS/nixpkgs#296769 for progress towards a cleaner API definition for packages.
There was a problem hiding this comment.
I've had a little breakthrough regarding this: NixOS/nixpkgs#312432
| * Further contributions that improve compliance with this RFC MAY involve | ||
| maintainers but SHOULD NOT be required to do so because the rules SHOULD be | ||
| clear. | ||
| * New packages MUST be compliant with the RFC (either no feature parameters or compliant with this RFC.) |
There was a problem hiding this comment.
A little something that hit me when I was experimenting with treating function args as an API in a different context:
| * New packages MUST be compliant with the RFC (either no feature parameters or compliant with this RFC.) | |
| * New packages MUST be compliant with the RFC (either no feature parameters or compliant with this RFC.) | |
| * New packages MUST NOT have attribute names that are compliant with feature parameter naming in order to not be automatically passed to packages expecting feature parameters. |
| or configure some compile-time parameter, like path to default mailbox or | ||
| initial size of hash map, derivation function (for short, package) MAY expose | ||
| compile-time options via feature parameters that match | ||
| `^(with|conf|enable)_[a-z_0-9]$` regular expression. For short, we will refer |
There was a problem hiding this comment.
It'd probably be good to explain this main proposal with a few more words for those who aren't fluent in regex ;)
While there are plenty of examples later of course, a few examples up here also wouldn't hurt.
| or configure some compile-time parameter, like path to default mailbox or | ||
| initial size of hash map, derivation function (for short, package) MAY expose | ||
| compile-time options via feature parameters that match | ||
| `^(with|conf|enable)_[a-z_0-9]$` regular expression. For short, we will refer |
There was a problem hiding this comment.
I don't really like the use of snake_case here. It intentionally breaks the widely used convention of camelCase in Nixpkgs.
|
Overall, I do not agree with @wolfgangwalther's proposal to add global parameter mechanism to this RFC or tie parameter naming to a global goal. The possibility of making such Gentoo-like USE-flags a reality is one fo the goals of this RFC as I understand it. Actually doing that should be explicitly excluded from this RFC though IMO. As I understand it, limiting it in this regard was intentional and I'd agree with that. A USE-flag like mechanism entails a lot of other design decisions and problems to solve than setting a standard on the naming of parameter names. This standardisation can be useful without the global mechanism and it'd be best to keep the RFC for it minimal. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/rfcsc-meeting-2024-05-14/45414/1 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/rfcsc-meeting-2024-05-28/46113/1 |
|
RFCSC: @wolfgangwalther: Asking again, would you be willing to shepherd this RFC? |
Sorry, for not answering so far. I had put this on the backlog, because nobody else was available to shepherd anyway, so it wasn't urgent for me. tbh, I don't know. This whole proposal is based on two key assumptions:
Both of those would allow superior solutions to this. But having no solution is clearly inferior. I only briefly looked at @Atemu's NixOS/nixpkgs#312432 - but this is an approach targeting a more principled solution. If that's feasible at all - we should pursue this instead. So I would say at this stage we can't reasonably push this RFC forward - to neither direction. Accepting this wouldn't make sense if @Atemu's approach works out well. Rejecting this now, doesn't make sense either, because we just don't have this other proposal anywhere close, yet. Imho, we should put this RFC on hold for a bit, maybe build a team to evaluate NixOS/nixpkgs#312432 and push this forward, possibly resulting in a different RFC. Once we have a better idea whether that approach can work, then re-consider this RFC here? |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/rfcsc-meeting-2024-06-10/46817/1 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/rfcsc-meeting-2024-06-24/47589/1 |
|
RFCSC: This RFC has not acquired enough shepherds. This typically shows lack of interest from the community. In order to progress a full shepherd team is required. Consider trying to raise interest by posting in Discourse, talking in Matrix or reaching out to people that you know. If not enough shepherds can be found in the next month we will close this RFC until we can find enough interested participants. The PR can be reopened at any time if more shepherd nominations are made. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/rfcsc-meeting-2024-07-08/48678/1 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/best-practices-for-config-flags-to-packages/4064/8 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
I don't find this particularly convincing. Having a 1:1 mapping isn't obviously beneficial. The only reason I can think of that would make it somewhat beneficial is if you're going to be doing automated parsing on attribute names. But in that case, you can easily make the first letter after the What about features that require multiple dependencies to be added at once, and where it doesn't make sense to have only one of the two optional dependencies? Currently, this RFC says there would need to be two parameters, I'm concerned that preferring |
Why "instead"? You can have both; |
What possible use case would there be for overriding the hypothetical |
|
Ah, that's a good point. Thinking about it again, I actually came to the conclusion that the defaults should work the other way around a while back (you can find it up above somewhere): The enable flag should default to the values of the individual with flags. Though that of course limits the effect of the enable flag to enabling/disabling the feature, not disabling the dependency; leaving it up to the build system to do the right thing. That should be expected though IMHO as only the |
|
Again though, why would you want to add a dependency if not to enable a feature? It would only add dead code paths. |
Maybe to replace the vendored version… |
|
I thought it was standard to to always replace the vendored version of things anyway? Regardless, I still find this an uncompelling use for
|
|
Of course replacement of dependencies adds integration bug risk, non-replacement increases the chances that some bug fix that is not in the vendored version is actually relevant to the specific use case, and no maintainer realistically has resources to check either part completely. Or both types of bugs are known to be there, known to require large changes to fix, and happen to be rare enough that for most use cases no more than one of these bugs is actually observable. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/rfcsc-meeting-2024-08-05/50170/1 |
RFC to establish rules on how
enable/with/support/useflags should be named. Essentially, an attempt to bring order and uniformity of Gentoo USE flags into Nixpkgs.Pre-RFC discussion on the discourse
Rendered RFC