-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
overlay: doc update for self/super -> final/previous #30958
Conversation
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.
There is a renaming issue: previous <-> final
e94fd38
to
7fb4800
Compare
doc/overlays.xml
Outdated
}; | ||
} | ||
</programlisting> | ||
|
||
<para>The first argument (<varname>self</varname>) corresponds to the final package | ||
<para>The first argument (<varname>final</varname>) corresponds to the previous package |
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.
Bad rewrite again :/
7fb4800
to
4ad8a3e
Compare
doc/overlays.xml
Outdated
corresponds to the result of the evaluation of the previous stages of | ||
<para>The second argument (<varname>previous</varname>) | ||
corresponds to the result of the evaluation of <literal>Nixpkgs</literal> with the | ||
overlays applied that are called prior to the application of the current overlay. |
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.
nit: s/that are called/
b05f34a
to
b0fab62
Compare
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.
Last nits before merging.
doc/overlays.xml
Outdated
}; | ||
} | ||
</programlisting> | ||
|
||
<para>The first argument (<varname>self</varname>) corresponds to the final package | ||
<para>The first argument (<varname>final</varname>) corresponds to the resulting | ||
package set after all the overlays are applied. | ||
set. You should use this set for the dependencies of all packages specified in your |
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.
nit: "… applied. set. You …"
doc/overlays.xml
Outdated
corresponds to the result of the evaluation of the previous stages of | ||
<para>The second argument (<varname>previous</varname>) | ||
corresponds to the result of the evaluation of <literal>Nixpkgs</literal> with the | ||
overlays applied prior to the application of the current overlay. | ||
Nixpkgs. It does not contain any of the packages added by the current |
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.
nit: "… current overlay. Nixpkgs. It …"
rr = super.callPackage ./pkgs/rr { | ||
stdenv = self.stdenv_32bit; | ||
rr = previous.callPackage ./pkgs/rr { | ||
stdenv = final.stdenv_32bit; |
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 isn't really final
though, is it? It can be overlayed on top of later.
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.
final
is the composition of all overlays, including the future ones too.
However, the brevity and clarity of final
in this rr
example highlights that we should use e.g. final.callPackage
, not previous.callPackage
for functions, and use previous
only when we explicitly want the previous definition (because the current definition will shadow it in the final set).
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.
@grahamc stdenv_32bit
is the final, as this is the result which went through the fix-point.
rr
is not final yet, it might be overriden, and would be in the previous
set of the next overlay.
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.
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, callPackage
is a function, and it should come from previous
, not final
.
fbc1979
to
b1b6b06
Compare
doc/overlays.xml
Outdated
}; | ||
rr = super.callPackage ./pkgs/rr { | ||
stdenv = self.stdenv_32bit; | ||
rr = final.callPackage ./pkgs/rr { |
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.
callPackage
is a function, which it-self captures self
to provide the packages to the package, and using it through self
will go through the fix-point twice.
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 does not make sense to me. For the interpreter the fix point is not a loop, and there is no such thing as the count of goes through the fix point. (Here is an accurate enough model of the internal states of a straightforward interpreter of the nix language that illustrates this: https://gist.github.com/orivej/5fddcf792dfa37a5bfa16d9a95fe70fa .) Using final
is in no way less computationally efficient than using previous
. When callPackage
is not overridden in a future overlay, final.callPackage
is exactly the same as previous.callPackage
, but the choice of the former is more obvious.
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.
@orivej Today there is not, but the reason the documentation is that way, is to make sure that we make the code written by user future-proof. Doing the opposite would become a problem for automatic grafting.
As you mentioned, there is no difference today, but doing it as currently mentioned in the documentation will prevent the addition of issues that would have to be fixed later.
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,
callPackage
is a function, and it should come fromprevious
, notfinal
.
Could you explain why functions should come from previous
? I am fairly certain that in practice this is a stylistic choice until someone wants to override some functions (and final
looks better), that we should use final
if they do, and it should not concern us if the referenced value is a derivation or a function . The only concern is if we can just use the final value, or if we need the previous value to implement our override of it.
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.
@nbp Could you detail the future plans? What is automatic grafting?
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.
From the name of it I guess the automatic grafting is deep merging of attribute sets, but that does not seem to cause any issues with using functions from final
.
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.
b1b6b06
to
5d5927f
Compare
@nbp ok, reverted. Look good now? |
corresponds to the result of the evaluation of the previous stages of | ||
<para>The second argument (<varname>previous</varname>) | ||
corresponds to the the evaluation of <literal>Nixpkgs</literal> with the | ||
overlays applied prior to the application of the current overlay. | ||
Nixpkgs. It does not contain any of the packages added by the current |
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.
nit: […] current overlay. Nixpkgs. It does […]
any updates? |
Beep boop, is this ready? |
I'm going to close this. |
Motivation for this change
confusion of what self and super represent in overlays, since this is something users can set and isn't hard coded, making this change to the docs should help.
@nbp
Things done
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)