lib.extendMkDerivation: add argument excludeFunctionArgNames and improve argument documentation#459124
Conversation
|
|
I'm curious when this will be useful. In most cases, we should be able to remove the deprecated arg from the destructured args and instead use |
Trivial build helpers, fetchers, and special build helpers typically avoid ellipses. The deprecated argument still needs to be present in the set pattern before hard removal. Without dedicated |
MattSturgeon
left a comment
There was a problem hiding this comment.
I have some feedback on the doc comment, but most of it predates this PR and can be addressed separately if preferred.
The typo should be addressed, but otherwise the actual changes LGTM.
|
I feel really queasy about backporting this change. Could you make a strong argument for changing/extending the function semantics two months before the branch goes quiescent? |
I only thought about build helper backporting when I added the label. I found no "strong arguments" supporting its backport. Let's drop the backporting label. |
philiptaron
left a comment
There was a problem hiding this comment.
LGTM % Matt's suggestions about the documentation.
| removeAttrs ( | ||
| # Inherit the __functionArgs from the base build helper | ||
| optionalAttrs inheritFunctionArgs (removeAttrs (functionArgs constructDrv) excludeDrvArgNames) | ||
| # Recover the __functionArgs from the derived build helper | ||
| // functionArgs (extendDrvArgs { }) | ||
| ) excludeFunctionArgNames |
There was a problem hiding this comment.
The semantics here, as I understand them:
- If
inheritFunctionArgsis set totrue(the default)
a. Whatever the arguments are toconstructDrvwill be used
b. Except for those listed inexcludeDrvArgNames, which will be excluded. - The arguments for the derived build helper will override that.
So ifconstructDrvhad an argument without a default, but the derived build helper had a default, the result would have a default. - Finally, no matter where it came from, if an argument is mentioned in the
excludeFunctionArgNameslist, it will be removed from the function arguments list.
I like those semantics.
I thought about adding the label myself (before I noticed it was already added). My argument being that core build helpers should (usually) have a consistent interface across supported releases, so that packages that use them can backport changes relying on said interfaces if needed. This is a semantically trivial change, so I'm comfortable with it being backported. However, on the flip side, future deprecations that use the new |
bff9174 to
ab49401
Compare
ab49401 to
3026703
Compare
excludeFunctionArgNamesargument tolib.customisation.extendMkDerivationto remove deprecated/does-not-mean-to-be-specified build helper arguments from__functionArgs.lib.functionArgscan be used as a way to check whether an argument is supported by a function. For example, nixpkgs-review useslib.functionArgsto detect ifpkgs.buildEnvsupports argumentignoreSingleFileOutputsfor Nixpkgs revision under review (see PR nix/review_shell.nix: use ignoreSingleFileOutputs whenever available Mic92/nixpkgs-review#427).excludeFunctionArgNamesprovides such mechanism.lib.extendMkDerivation.Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.