-
-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
lib.extendMkDerivation, lib.adaptMkDerivation: init #234651
base: master
Are you sure you want to change the base?
Conversation
a2bd999
to
301bd5b
Compare
I should have checked more carefully. There's a function called Updat: fixed, see below. |
301bd5b
to
4f7af7a
Compare
Fix |
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/avoid-rec-expresions-in-nixpkgs/8293/18 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A mix of incremental and fundamental suggestions, because I can't decide for the nixpkgs-python maintainers.
I'm concerned about the increasing complexity, while the goal can be achieved through simplification instead: removing the python-specific level of overriding and turning it into an "overlay" on the mkDerivation
arguments instead.
The alternative strategy is to
- Provide this python layer, which contains the relevant
mkPyton*
logic in a way that works with overlay-style overriding. This can be done by reading the existing code, attribute for attribute, and adding the logic to the python layer. - Change the python packages to use that layer in combination with
mkDerivation
, instead of the currentmkPython*
functions. - Perhaps make the
mkPython
functions reuse the overlay so that they don't literally reimplement the same logic. I don't know if this would be worthwhile. - Eventually deprecate the
mkPython*
functions.
I have played around with step 1 of the alternative strategy. It is feasible, but it requires a bit of migration for each package. I'm not a nixpkgs-python maintainer, so I don't feel like I should be the one to make the decision whether to accept the complexity of this PR, or refactor to actually simplify the python logic by fitting it into a mkDerivation
layer.
doc/builders/builders.chapter.md
Outdated
}: | ||
|
||
lib.extendRecursiveBuilder stdenv.mkDerivation [ ] (finalAttrs: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth noting that finalAttrs
contains the final arguments to mkDerivation
attrs, and not the final arguments to the function you're constructing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the arguments after applying the modifier
function is also considered.
IMO, the finalAttrs
is meant to be the final state of the attributes passing to the base builder (mkDerivation
). That's how user could access finalAttrs.finalPackage
and other goodies. If we just want to get the input argument set manually, the user could just lib.fix
the function themselves.
Input arguments that doesn't mean to be passed to the base builder are subject to special care, and should never enter the recursion. Those not-to-pass arguments are also the reason why builder overlays are not drop-in replacement of current builders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to add those special arguments to finalAttrs
, but that greatly increase the complexity of the code and makes behavior surprising (the specified result will be different from what is got through finalAttrs
by design).
A better way would be to encourage passing all the arguments. When all the arguments are passed, they will all be available inside finalAttrs
, and we could then switch to the overlay-based workflow specification (from the current, build-helper-based one).
in | ||
if builtins.isAttrs result then result | ||
else if builtins.isFunction result then { | ||
overridePythonAttrs = newArgs: makeOverridablePythonPackage f (overrideWith newArgs); | ||
__functor = self: result; | ||
} | ||
else result; | ||
else result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels too custom or repeated (not sure yet which). Does this add mkDerivation
-like fixpoint logic at the python attrs level?
I believe we should merge the python attrs level into the mkDerivation attrs, so that the interface and implementation become simpler. Having multiple levels of overriding has a huge complexity cost, so getting rid of an unnecessary level would be a huge win. We'd get rid of overridePythonAttrs
and all the user facing complexity, implementation complexity and bugs that come with it.
The python-specific attrs can almost be implemented as an "overlay" on the mkDerivation
arguments. When I tried this, I think only like 3 attributes had the same name but a slightly different meaning. That made it a breaking change, but migrating those attributes is feasible and would vastly simplify the python/mkDerivation wiring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With same names that is generally speaking because the Python builder will extend the lists with some "defaults". That's really the only value of the custom builder over just plain hooks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels too custom or repeated (not sure yet which). Does this add
mkDerivation
-like fixpoint logic at the python attrs level?
I personally prefer passing most of the attributes into mkDerivation
, encourage the use of overrideAttrs
and gradually deprecates overridePythonAttrs
as well as other builder-specific override methods.
That will be a mass rebuild, so I just work around the makeOverridablePythonPackage
obstacle in order to demonstrate the possibility to add the recursive attributes support without rebuild.
Recently the Packages Modules Working Group started investigating whether something like the module system could be used to evaluate packages. We're tracking all work in this repository, meeting weekly, but working mostly asynchronously. It would be great if you could join the Matrix room of the WG and chat with us, or even join the team yourself to work on such issues! |
Thank you for taking time reviewing this!
The idea to shift workflow-specific overlays sounds neat, and that could also be friendlier when packaging multi-language projects. Nevertheless, there are some issue on the way to the switch:
Overall, the goal of the proposed function is to add |
doc/builders/builders.chapter.md
Outdated
@@ -0,0 +1,42 @@ | |||
# The builders {#chap-builders} | |||
|
|||
A Builder, in the context of Nixpkgs, are functions that produces derivations. (Don't confuse it with the `builder` input argument of function `derivation`, which refers to the path to the executable that builds the derivation.) Such function is usually designed to follow a specific workflow, so that one can produce a package by specifying a few options instead of messing up with the `derivation` function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Builder, in the context of Nixpkgs, are functions that produces derivations. (Don't confuse it with the `builder` input argument of function `derivation`, which refers to the path to the executable that builds the derivation.) Such function is usually designed to follow a specific workflow, so that one can produce a package by specifying a few options instead of messing up with the `derivation` function. | |
A *builder*, in the context of Nixpkgs, is a function that produces derivations. | |
:::{.warning} | |
This is not to be confused with the [`builder` argument to `derivation`](https://nixos.org/manual/nix/unstable/language/derivations.html), which refers to the executable that produces the build result. | |
::: | |
Such a builder is usually designed to follow a specific workflow, so that one can produce a package by setting a limited set of options relevant to the particular use case instead of using the `derivation` function directly. |
This is terrible naming. The least we can do is highlight it with red alarms (not sure about admonition syntax, it's all different everywhere...) Since you're going at great lengths to improve documentation (which is absolutely awesome!) we may as well change that while at it. I'd certainly support it by bouncing around proposals. But of course feel free to ignore requests to widen scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible candidates:
-
Build function
-
Existing use referring to the same thing
-
androidenv.buildApp
doc/languages-frameworks/android.section.md
## Building an Android application {#building-an-android-application}
This build function is particularly useful when it is desired to use
Hydra: the Nix-based continuous integration solution
to build Android apps. -
xcodeenv.composeXcodeWrapper
doc/languages-frameworks/ios.section.md
## Deploying a proxy component wrapper exposing Xcode {#deploying-a-proxy-component-wrapper-exposing-xcode}
The first use case is deploying a Nix package that provides symlinks to the Xcode
installation on the host system. This package can be used as a build input to
any build function implemented in the Nix expression language that requires
Xcode.
-
-
Existing use referring to something else
- Inside the build process of
pkgs/build-support/rust/build-rust-crate/build-crate.nix
# configure & source common build functions
- Inside the build process of
-
-
Build helper
-
Existing use referring to the same thing
buildIntegration
pkgs/tools/networking/dd-agent/integrations-core.nix
# Build helper to build a single datadog integration package.
-
Existing use referring to something else
- Inside the
meta.description
of the Haskell Hackage packagehein
pkgs/development/haskell-modules/hackage-packages.nix
An extensible build helper for haskell, in the vein of leiningen
- Inside the
-
The different use of both candidates doesn't seem to cause confusion. We could just choose from one of them, or come up with something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great research. I like "build helper" more, because its closest to what the thing is about, and also "build function" only contains one word that adds information (almost everything is a function).
When we change section titles we should make sure URLs stay stable. Either keep the anchors or add redirects (we discussed this with @pennae somewhat recently but I can't remember why the script used in the Nix manual was not included).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we discussed this with @pennae somewhat recently but I can't remember why the script used in the Nix manual was not included
there's not technical reason it couldn't be done, it's just that nobody has done the work yet. adding first-class support for fragment redirects to nixos-render-docs would be nicer and less error-prone, but we don't have to do that as a first step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree that we should rename this from builder to something else and that "build helper" is better. Quite the contrary, the function when called actually builds something, which I'd say is exactly what a builder is. This naming has been around for a very long time.
The Nix attribute builder
is a detail that no typical user would ever encounter. Those interested going through say the Pills or in detail through the Nix manual will see it, but otherwise unless you're interested in writing a custom stdenv.mkDerivation
you won't do anything with it. Hence, I don't think that part should even be mentioned here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nix language functions don't build anything, not even the derivation
primitive. What we typically mean by "build" is to run some process to produce files from other files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following that line of reasoning "build helper" is also incorrect, because we're solely preprocessing before deriving. So in that case "deriver" or "deriver helper" or "derivation preprocessor" or "build planner" is more correct. Though actually they invoke derivation
as well so they're not solely peprocessors either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearly this needs discussion, but I don't want to block this PR on a single term. I'm happy with "build helper" because it reduces ambiguity and is not obviously wrong, and I'm not opposed to further improving the terminology in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could move the builder
naming to another issue or PR.
BTW, "builders" also refers to "build machines" in the context of "remote builders", which is probably why the documentation of darwin.builder
is misplaced under the "Builders" part in the Nixpkgs manual. It would be great to find a more suitable place for it. (#235858)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we ever have a user be confused about these being called builders? I at least haven't, and I don't think we should be even spending time discussing this unless there are users confused about it.
doc/builders/builders.chapter.md
Outdated
} | ||
``` | ||
|
||
A list of functions to modify the resulting derivation can be specified after the base builder. Modifications such as overriding and `extendDerivation` can be applied there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do we find information about extendDerivation
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib.customisation
is currently not presented in the Nixpkgs manual. We'll need to add the documentation for those functions before referring to them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides, the Nixpkgs Library Functions documentation automatically generated from the comment don't have anchors. So there's no way to link against them so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My fault. The anchor in the form function-library-<long attribute path>
is also automatically generated. E. g. function-library-lib.customisation.extendMkDerivation
.
This usage is very hard to discover, as the documentation is missing, and the manual provides no hyperlink in the title of each function document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I addressed this review in the changes to add a new section about using lib.extendMkDerivatien
to define build helpers. Please take a look.
Just reviewing your change to input-remapper, I like how much this cleaned up its definition. :) |
954f48e
to
2ee39a7
Compare
f3c34fb
to
d868837
Compare
lib/customisation.nix
Outdated
adaptArgs: | ||
mirrorFunctionArgs | ||
# Make the __functionArgs looks like one from a build helper accepting plain attribute set. | ||
(adaptArgs { }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not compose well.
There are three build helpers for Emacs lisp packages: melpaBuild, elpaBuild and trivialBuild. These three build helpers all share one base build helper: genericBuild.
let
genericBuild = adaptMkDerivation stdenv.mkDerivation (finalAttrs: { ... }@args: { ... })
in
melpaBuild = adaptMkDerivation genericBuild (finalAttrs: { ... }@args: { ... })
Use melpaBuild
as an example. It receives arguments of its own like files
and arguments of genericBuild
like packageRequires
. So ideally functionArgs melpaBuild
should contains these two kinds of arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the __functionArgs
of derived build helpers don't include those from their based build helpers due to the nature of lib.functionArgs
.
It's possible to add it as a new feature, which would be great for extendMkDerivation
. However, it might not be a good idea for adaptMkDerivation
- derived build helpers to inherit __functionArgs
from the base build helper, as the primary goal of adaptMkDerivation
is to allow attribution removal before passing to the base build helper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible to add it as a new feature, which would be great for
extendMkDerivation
.
Update:
Such a feature would also be surprising if the derived build helper doesn't have ellipses as an edge case.
Most mkDerivation
-like build helpers have ellipsis to accept arbitrary arguments and pass them as derivation attributes. It would benefit them to inherit __functionArgs
from the base build helper.
Trivial build helpers (defined in pkgs/build-support/trivial-builders/default.nix
) usually avoid ellipses and take attributes from derivationArgs
instead. Still, such situations would fall into the adaptMkDerivation
use cases if support for fixed-point arguments is desired.
I like the idea of __functionArgs
inheritance implemented in extendMkDerivation
. Do I miss any other edge cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the
__functionArgs
of derived build helpers don't include those from their based build helpers due to the nature oflib.functionArgs
.
Could you elaborate on what nature of lib.functionArgs
prevents derived build helpers from getting __functionArgs
of their base build helpers? I think it is doable if (adaptArgs { })
is changed to something like this:
(lib.setFunctionArgs
(_: _)
(lib.functionArgs mkDerivationBase // lib.functionArgs (adaptArgs { })))
It's possible to add it as a new feature ... Such a feature would also be surprising if the derived build helper doesn't have ellipses as an edge case.
I agree. What about adding a new argument to lib.extendMkDerivation
and friends to control their behavior? This new argument is an attribute set and currently it has only one attribute: { inheritArgsFromBase ? true }
. Note that this attribute set leaves space for future extension.
it might not be a good idea for
adaptMkDerivation
- derived build helpers to inherit__functionArgs
from the base build helper, as the primary goal ofadaptMkDerivation
is to allow attribution removal before passing to the base build helper.
I do not follow. Why is attribution removal relevant here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the
__functionArgs
of derived build helpers don't include those from their based build helpers due to the nature oflib.functionArgs
.Could you elaborate on what nature of
lib.functionArgs
prevents derived build helpers from getting__functionArgs
of their base build helpers?
By saying "currently," I mean "before the merge of this PR." Current build helpers take plain attribute sets, and the current way of defining new build helpers is to write new attribute-set-taking functions. The build helpers' current __functionArgs
do not include the ones from their base build helpers.
I do not follow. Why is attribution removal relevant here?
We could already define new build helpers taking fixed-point arguments by passing all the user-specified arguments down to the base build helper and exploiting the <pkg>.overrideAttrs
of the resulting package. extendMkDerivation
is essentially a wrapper around <pkg>.overrideAttrs
.
This method only works when defining new build helpers rather than when migrating existing ones because the existing ones only pass part of the set of arguments down the base build helpers. It's easy to pass the arguments from plain attribute sets selectively but challenging when taking fixed-point arguments ((finalAttrs: { })
).
We could solve the issue by refactoring the current build helpers and passing each argument properly, which often implies mass rebuilds and long review time. To speed up the adoption of fixed-point arguments support, this PR introduces another function, adaptMkDerivation
, as a quick and no-rebuilt workaround for the above issue.
As some of the arguments might be removed before passing to mkDerivationBase
when using adaptMkDerivaiton
, inheriting the __functionArgs
from the mkDerivationBase
with adaptMkDerivaiton
would be unsuitable. In the long run, we should encourage the migration from adaptBuildHelper
to extendBuildHelper
for non-trivial build helpers to simplify overriding and eliminate the use of custom overriders (e.g., overridePythonAttrs
).
It's possible to add it as a new feature ... Such a feature would also be surprising if the derived build helper doesn't have ellipses as an edge case.
I agree. What about adding a new argument to
lib.extendMkDerivation
and friends to control their behavior? This new argument is an attribute set and currently it has only one attribute:{ inheritArgsFromBase ? true }
. Note that this attribute set leaves space for future extension.
Taking an additional attribute set to configure the behavior of extendMkDerivation
is a solution. Still, it seems a bit involved always to add an extra { }
to make the __functionAttrs
prettier.
I'm considering adding the __functionArgs
inheritance to extendMkDerivation
. People could still choose adaptMkDerivation
or the manual <pkg>.overrideAttrs
should there be any edge case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the __functionArgs
inheritance to extendMkDerivation
. Please take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed explanation!
I should have been more clear about what I do not follow. I understand the motivation for lib.adaptMkDerivaiton
well.
As some of the arguments might be removed before passing to
mkDerivationBase
when usingadaptMkDerivaiton
, inheriting the__functionArgs
from themkDerivationBase
withadaptMkDerivaiton
would be unsuitable.
What I do not quite get is the logic behind this statement. Now after thinking about it for a while, I figure it out. If the base build helper has an argument with the same name as the argument of the derived build helper you want to remove, then not inheriting arguments may be better. However, in other cases, I think it is better to do the inheritance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for explaining. I must elaborate on the relation between attribute removal and its influence on the preference to/not inherit __functionArgs
from the base build helper.
You're right. The argument removal doesn't make it surprising to inherit __functionArgs
from the base build helper. The inheritance would be unexpected when the developer of the derived build helper wants people to refrain from using specific arguments from the base build helper.
Take buildPythonPackage
and buildPythonApplication
for example. For historical reasons, the buildPyton*
build helpers take checkPhase
-related arguments to determine the installCheck
behaviors. Renaming the arguments to the installCheckPhase
-related ones would make them more correct, but such correctness alone doesn't justify the painful mass renaming. As a result, buildPython*
build helpers ask the user to stick to the checkPhase
-related arguments and avoid specifying the installCheckPhase
-related ones. Assuming that someday stdenv.mkDerivation
provides __functionArgs
for all its documented attributes, the installCheckPhase
-related arguments would also appear in the __functionArgs
of buildPythonPackage
if adaptMkDerivation
does the inheritance.
A counterargument would be that keeping a specific argument inside the function set pattern does not necessarily imply endorsement of such an argument. For instance, buildGoModule
keeps buildFlags
and buildFlagsArray
in its set pattern but explicitly discourages its specification with evaluation warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I just forgot the ellipsis. Non-'mkDerivation'- like build helpers— trivial build helpers, fetchers, etc.—usually use derivationArgs
instead of ellipsis. They need adaptMkDerivation
to remove derivationArgs
while taking fixed-point arguments. Whether or not we should add fixed-point arguments support to trivial build helpers is debatable, but it's good to leave some room for such support.
An extra configuration attribute set might be a good idea. If only we have builtins.functionInfo
(NixOS/nix#8961 or NixOS/nix#8908) or something like that!
9e878f9
to
19c8904
Compare
19c8904
to
dad6818
Compare
Would it be desirable to add an extra attribute set to configure the behavior of lib.adaptMkDerivation { modify = ...; } stdenv.mkDerivation (finalAttrs:
{ preferLocalBuild ? true
, allowSubstitute ? false
, impassableArg ? (x: false)
, ...
}@args:
{
inherit preferLocalBuild allowSubstitute;
# Some other build process
} // removeAttrs args [ "impassableArg" ]) |
b0aeaf8
to
486f966
Compare
I also adopted the relevant format for writing comment block documentation for |
lib/customisation.nix
Outdated
) | ||
// { | ||
# Passthru attributes attached to the result build helper. | ||
inherit attrsOverlay; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know the motivation and use case of exposing attrsOverlay
. Considering composability only, should the exposed overlay take mkDerivationBase
into consideration? Something like
inherit attrsOverlay; | |
attrsOverlay = lib.composeExtensions mkDerivationBase.attrsOverlay or { } attrsOverlay; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I exposed attrsOverlay
so that people could experiment with the idea of applying attribute overlays to overrideAttrs
proposed by Robert in the review comment:
Considering composability, maybe we could change it to attrsOverlays
?
{
attrsOverlays = mkDerivationBase.attrsOverlays or [ ] ++ [ attrsOverlay ];
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After more consideration, I think it would be easier to expose only attrsOverlay
and mkDerivationBase
. We could add more later if need be.
486f966
to
e70488e
Compare
( | ||
# Inherit the __functionArgs from the base build helper | ||
optionalAttrs inheritFunctionArgs (functionArgs mkDerivationBase) | ||
# Recover the __functionArgs from the derived build helper | ||
// functionArgs (attrsOverlay { }) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
( | |
# Inherit the __functionArgs from the base build helper | |
optionalAttrs inheritFunctionArgs (functionArgs mkDerivationBase) | |
# Recover the __functionArgs from the derived build helper | |
// functionArgs (attrsOverlay { }) | |
) | |
( | |
# Recover the __functionArgs from the derived build helper | |
functionArgs (attrsOverlay { }) | |
# Inherit the __functionArgs from the base build helper | |
// optionalAttrs inheritFunctionArgs (functionArgs mkDerivationBase) | |
) |
Note that overlay-a
is before overlay-b
(the reverse order of adaptMkDerivation
).
extendMkDerivation { } (extendMkDerivation { } stdenv.mkDerivation overlay-a) overlay-b
Another question: If overlay-a
sets one argument foo
of functionArgs (overlay-b { })
and foo
is not in functionArgs (overlay-a { })
, should that argument foo
be removed from the result __functionArgs
? A similar question also applies to adaptMkDerivation
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should that argument foo be removed from the result __functionArgs?
Ideally, yes. However, there's no programmatic way to achieve such removal.
886a96a
to
b30a617
Compare
I rebased the branch to resolve merge conflicts, restructured the |
Add functions to lib.customisation: - extendMkDerivation - adaptMkDerivation Co-authored-by: Robert Hensing <[email protected]> Co-authored-by: Valentin Gagarin <[email protected]> Co-authored-by: Lin Jian <[email protected]>
Add "Fixed-point arguments of build helpers" chapter in "Builde helpers" part. Co-authored-by: nicoo <[email protected]> Co-authored-by: Silvan Mosberger <[email protected]> Co-authored-by: Valentin Gagarin <[email protected]> Co-authored-by: Lin Jian <[email protected]>
b30a617
to
1f0fa69
Compare
Previously, we vendor PR NixOS#234651 because we want to keep the old behavior of filtering out packageRequires from the arguments we pass to the underling stdenv.mkDerivation. Doing so raises the concern about the complexity of PR NixOS#234651. Considering that passing packageRequires to stdenv.mkDerivation also works well, we stop filtering it out and stop vendoring PR NixOS#234651. Now, this PR only uses the existing interface of stdenv.mkDerivation. Even though the name of the build helper is still extendMkDerivation', it is nothing new and has been used in Nixpkgs, such as php.buildComposerProject[1]. [1]: https://github.com/NixOS/nixpkgs/blob/f3834de3782b82bfc666abf664f946d0e7d1f116/pkgs/build-support/php/builders/v1/build-composer-project.nix#L108
Previously, we vendor PR NixOS#234651 because we want to keep the old behavior of filtering out packageRequires from the arguments we pass to the underling stdenv.mkDerivation. Doing so raises the concern about the complexity of PR NixOS#234651. Considering that passing packageRequires to stdenv.mkDerivation also works well, we stop filtering it out and stop vendoring PR NixOS#234651. Now, this PR only uses the existing interface of stdenv.mkDerivation. Even though the name of the build helper is still extendMkDerivation', it is nothing new and has been used in Nixpkgs, such as php.buildComposerProject[1]. [1]: https://github.com/NixOS/nixpkgs/blob/f3834de3782b82bfc666abf664f946d0e7d1f116/pkgs/build-support/php/builders/v1/build-composer-project.nix#L108
Previously, we vendor PR NixOS#234651 because we want to keep the old behavior of filtering out packageRequires from the arguments we pass to the underling stdenv.mkDerivation. Doing so raises the concern about the complexity of PR NixOS#234651. Considering that passing packageRequires to stdenv.mkDerivation also works well, we stop filtering it out and stop vendoring PR NixOS#234651. Now, this PR only uses the existing interface of stdenv.mkDerivation. Even though the name of the build helper is still extendMkDerivation', it is nothing new and has been used in Nixpkgs, such as php.buildComposerProject[1]. [1]: https://github.com/NixOS/nixpkgs/blob/f3834de3782b82bfc666abf664f946d0e7d1f116/pkgs/build-support/php/builders/v1/build-composer-project.nix#L108
Description of changes
This PR introduces a unified approach to implement build helpers that support fixed-point arguments (
lib.extendMkDerivation
) and bring such support to existing build helpers (lib.adaptMkDerivation
).The fixed-point arguments support in
stdenv.mkDerivation
is introduced in #119942, and there are ongoing attempts to make other build helpers support such syntax (buildRustPackage
refactor #194475,buildGoModule
refactor #225051). The overlay styleoverrideAttrs
brought by thestdenv.mkDerivation
change can be used to implement the functionality, which is adopted by thebuildRustPackage
refactor and the previous version of thebuildGoModule
refactor. The challenge of such an approach is that the whole set pattern matching the input arguments are degenerated into a single identifier, making it hard to see from the source code which attributes to the build helper accepts.The new Nixpkgs Library function,
lib.extendMkDerivation
accepts a base build helper and an attribute overlay (an overlay in the formfinalAttrs: args: <attrsToUpdate>
), and returns a new build helper by extending the base build helper with the attribute overlay via its<pkg>.overrideAttrs
.The following is the definition of an example build helper,
mkLocalDerivation
:For existing build helpers with arguments that cannot be passed to the base build helper, another Nixpkgs library function,
lib.adaptMkDerivation
, is provided. Instead of an attribute overlay, it takes a function that turns the arguments of the result build helper to those of the base build helper, known as an argument set adapter. This way, developers can decide not to pass some arguments down to the base build helper.The reason to have two functions instead of just
lib.adaptMkDerivation
, is that we want to encourage (new) build helpers to pass all their input arguments properly down tostdenv.mkDerivation
, and provide override functionality through the standardized<pkg>.overrideAttrs
instead of custom overriders such asoverridePythonAttrs
,overrideNimAttrs
, etc, by encouraging the use oflib.extendMkDerivation
. In the meantime, providinglib.extendMkDerivation
-incompatible build helpers with fixed-point arguments support (bylib.adaptMkDerivation
) enables a smoother transition to alib.extendMkDerivation
-compatible style.Fewer expression changes is another plus to convert existing build helpers with
lib.adaptMkDerivation
to take fixed-point arguments. Working build helper to support recursive attributes. A subsequent PR #271387 enables the fixed-point arguments support inbuildPythonPackage
andbuildPythonApplication
with slight modification againstmk-python-derivation.nix
with no rebuilds.Both
lib.extendMkDerivation
andlib.adaptMkDerivation
take a set of optional arguments to manipulate their behaviors. For examplelib.adaptMkDerivation { modify = drv: toPythonModule drv; }
appliestoPythonModule
to the derivation produced by the derived build helper.Cc:
Python maintainers: @FRidh @mweinelt @jonringer
Author of the
buildRustPackage
refactor PR @amesgenReviewer of the
buildGoModule
refactor PR @zowoqAuthor of the merged recursive
stdenv.mkDerivation
PR @roberthPeople who mention 119942 in Python application definition: @LunNova
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)