-
-
Notifications
You must be signed in to change notification settings - Fork 160
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
[RFC 0003] SOS: Simple Override Strategy. #3
Conversation
I do not fully understand the issue here regarding memory consumption. I have created a gist at https://gist.github.com/bennofs/77ebe3cf148f8cc4ebfd5fa5edc9e412 declaring a few packages with old-style vs new-style. Let's walk through evaluation of old-styleFirst, we evaluate the After that, for building, nix will take the Note that new-styleFirst, we evaluate Next, we need to Am I overlooking something? Do those memory problems only occur when you actually use overrides? |
@bennofs I think the memory issues are related to overrides. In that case both the original and overridden values are kept in memory. It can be an issue especially on the $lang package sets where each package is overridden with a specific $lang version. |
@zimbatm that makes some more sense. Say we do the following:
Then, when evaluating |
@nbp is it possible to instrument nix to get a better understanding of runtime memory usage? I think an example to illustrate the problem would be quite helpful. |
Some packages have a generic argument, that needs to be passed when calling callPackage. How would you handle that ? |
@nbp We should finally get around to figuring the out cross and bootstrapping parts of this plan :). |
@nbp It looks to me like the actual build plan is missing from the specification of packages; how would I specify that |
Mostly yes. The This proposal will move the override part, such that it is done before the calls to This will also definitely help
Instrumenting Nix would be quite painful to do, especially identifying the attribute which are corresponding to the override. Instead I did something simpler, which is to instrument the override functions of Nixpkgs. Instrumentation: nbp/nixpkgs-2@bd90e8a
|
Taking for example qca2 [1], this would be translated into: {
qca2 = import ../development/libraries/qca2 self super /</ { buildDeps.qt = />/ self.qt4; };
qca2-qt5 = import ../development/libraries/qca2 self super /</ { buildDeps.qt = />/ self.qt5.qtbase; };
} Where Basically, this is as-if we had rewritten the full set, but only changed the |
The idea is that each expression must provide a The top-level of Nixpkgs is in charge of calling the |
That’s awesome, yes. Build instructions instead of actually building at the lowest level, as e.g. in Haskell’s |
How will conditionals work? Especially those that depend on whether a dependency is configured with a certain feature. I'd think we'd need the dependency in scope over the whole attribute set then. |
''; | ||
}; | ||
} | ||
``` |
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.
Taking a deepMerge approach would look like this:
self: super:
{
fortytwo_1_0 = let parent = super.fortytwo_1_0; in deepMerge(parent, rec {
buildDeps.foo = self.foo_1_21;
buildPhase = ''
${parent.buildPhase}
other_command ${buildDeps.foo}
'';
});
}
I suppose the issue is that if the package is extended again, buildDeps.foo
would be overridable. Is that the issue that you are trying to address?
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.
The problem is that at some point you may want to stop joining, and start overriding.
set
# gives
{
a = {
aa = "a-aa";
bb = "a-bb";
};
b = {
aa = "b-aa";
bb = "b-bb";
};
}
lib.recursiveUpdate set { a = { cc = "a-cc"; }; }
# gives
{
a = {
aa = "a-aa";
bb = "a-bb";
cc = "a-cc";
};
b = {
aa = "b-aa";
bb = "b-bb";
};
}
set /</ { a = />/: { cc = "a-cc"; }; }
# gives
{
a = {
cc = "a-cc";
};
b = {
aa = "b-aa";
bb = "b-bb";
};
}
This is a problem if you want to override buildInputs.selenium.browser
(not a real example) from firefox to chromium. With lib.recursiveUpdate
, you will end up with buildInputs.selenium.browser == lib.recursiveUpdate firefox chromium
which would give really strange results. You want to get buildInputs.selenium.browser == chromium
. This is a deep update until you reach the browser
attribute, at which point you stop recursing and replace the old value by the new one, without merging.
You would write it ... /</ { buildInputs.selenium.browser = />/: chromium; }
.
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 suppose the issue is that if the package is extended again,
buildDeps.foo
would be overridable. Is that the issue that you are trying to address?
Yes, this is part of the issues that are addressed by making buildPhase
a function.
The other issue is the one described by @layus, which is that we want to stop overriding at foo
, instead of within foo
. The difference being that if we stop within foo
with might have extra information carried by the original expression into the result, thus leading a different evaluation.
I get the sense that this approach looks simpler on the surface but will be harder to understand one overrides or conditionals are added to the mix. Happy to be proved wrong. |
It seems much simpler to just add a section to the manual explaining in detail how the various overrides can be done. The operators seem to be invented by someone who just likes to program Haskell. Nothing wrong with Haskell, but it's just biased. In short, I am not in favor of this feature. |
Is it necessary to switch all of nixpkgs at the same time or could a proof-of-concept be implemented for a subset? I think seeing it work and implement a couple of package will help better get a sense of if it's an improvement or not. |
Incremental should work fine. |
This is part of the current proposal. |
Maybe not an urgent concern, but I have always been bothered by the two ways to write attrsets in nix: A special builtin (say self: super:
{
fortytwo_1_0 = super.fortytwo_1_0 /<>/ {
buildDeps.foo = self.foo_1_21; # stop recursing after the '='.
};
} This is in general what people expect when overriding. They name the (sub)attribute to update, and provide a value. From the nixpkgs manual, we have this example let
pkgs = import <nixpkgs> {};
newpkgs = import pkgs.path { overlays = [ (pkgsself: pkgssuper: {
python27 = let
packageOverrides = self: super: {
numpy = super.numpy_1_10;
};
in pkgssuper.python27.override {inherit packageOverrides;};
} ) ]; };
in newpkgs.inkscape it could be turned into let
pkgs = import <nixpkgs> {};
newpkgs = pkgs /<>/ {
python27.numpy = newpkgs.python27.numpy_1_10;
};
in newpkgs.inkscape |
@0xABAB , are you comparing the complexity of the implementation against the complexity of the usage? Constructing something out of state which appear to be declarative is easier to understand, than computing something which seems to be made up of abstract parameters, unless you are a developer. This override approach should lower the level of accessibility of packaging, while improving nix-env listing of packages, while removing intermediate functions. All that by moving the complexity cost into the implementation of the operators instead of being into the users code. |
Then the semantic of your operator will change based on the definitions of the attributes, which implies that semantic of the operator depends on something which is not locally visible: ( ((let x = { b = "c"; c = true; }; in { a = x; }) /<>/ { a.b = "d"; }).a.c or false)
!=
({ a = { b = "c"; c = true; }; } /<>/ { a.b = "d"; }).a.c |
Another question: some |
I do not see any issue why it would not work the same as before: self: super:
{
fortytwo_1_0 = super.fortytwo_1_0 /</ {
buildDeps.foo = />/: self.foo_1_21;
configureFlags = />/@configureFlags: with : // { inherit configureFlags; };
configureFlags ++ lib.optional (foo != null) "--with-foo=${foo}";
};
} |
@nbp I looked at the proposal and I wasn't thrilled by what it would buy me. Now, I am a developer, so that might be one of the reasons. I would present a real non-trivial example for this feature, compare that to the current version and just spam a couple of random users via GitHub (e.g. in a reply when you help them with something else). If it then is the case that they say like "Wow, that's what I have been waiting for", then there might be a reason to implement this, but until that time it appears to be a solution looking for a problem. If we compare this RFC with the one about encryption, then it's clear there is a "demand" for it. |
In your previous example: let
pkgs = import <nixpkgs> {};
newpkgs = pkgs /<>/ {
python27.numpy = newpkgs.python27.numpy_1_10;
};
in newpkgs.inkscape You will still require to have an overlay, otherwise the changes won't apply to inkscape. |
@layus, unrelated to this PR, but in the following example: let
pkgs = import <nixpkgs> {};
newpkgs = import pkgs.path {
... the first import is not necessary to resolve let
newpkgs = import <nixpkgs> {
overlays = [ (self: super: super /<>/ { python27.numpy = ...; } ) ];
};
in newpkgs.inkscape |
There is no reason to prioritize RFCs. Everything can be worked on in parallel by different people. @nbp has been chipping away at closely related problems since early 2015, as you can see in his Nixcon ’15 presentation. |
@nbp with separate lambdas for configure flags and dependencies, I worry some logic would have to be duplicated or other abstraction needed. I do like the drv builder part, or @Profpatsch's old to string field idea which is similar. Maybe we can start with just that? |
@nbp I wrote a document with Nix language ideas: https://gist.github.com/edolstra/29ce9d8ea399b703a7023073b0dbc00d. It has quite a bit of overlap with SOS. |
I think I really like the library refactor part of SOS and @edolstra's proposal, but would like to try them before adding new language features. I think if we had a (polymorphism-supporting) type system, the idea of so many more fix points without dedicated language support would be less daunting too. Finally, I am skeptical of not needing super. With Haskell package sets today we already commonly do Most importantly though, thank you for taking the time to design and write up what is undoubtedly a fascinating and compelling vision of the future, @edolstra! |
@edolstra , really nice proposal, and much better syntax. I have multiple comments:
|
(writing this before reading @nbp's comment above) I have a pure library implementation of the language features described here (except include, which can be built on top of them) at https://github.com/shlevy/nixpkgs/tree/extensible-records. Library is at https://github.com/shlevy/nixpkgs/blob/88061e59f5336866c8aa6e8cf1231558248f6b33/lib/extensible-records.nix , examples/tests at https://github.com/shlevy/nixpkgs/blob/88061e59f5336866c8aa6e8cf1231558248f6b33/lib/tests/extrec.nix . Note that, as described in the top of |
@Ericson2314 ^ FYI |
@nbp responding to some of your comments in light of my library impl and my idea about assignment semantics.
I think to pass different arguments you can just use
My answer to this is that you can do
How I'd do this with my syntax: let overlay1 = {{ hello = package {{ }}; guiHello = package {{ inherit hello; }}; }};
overlay2 = {{ hello_fork = hello with {{ hello := hello; }}; guiHello = guiHello with {{ hello := hello_fork; }};
overlay3 = {{ hello = {{ name = "goodbye"; }}; }}; Note that the
Agreed.
Yes, I think Eelco explicitly forbade this.
Agreed, though maybe it would be good then to have a version of
I'm not completely sure I understand what you mean here, but I think this issolved by recursing only into |
The whole reason why I started this SOS work, was to make it possible to do automatic grafting, as required by NixOS/nixpkgs#10851 , while getting rid of all inner fix-points and having a clear set of inputs to compare between the non-grafted version and with the grafted version. After, I also saw the opportunity to unify all the override functions, by moving the arguments and builder in the attribute set it-self, and reducing the computation time, and the property lookup of metadata. I would be really sad if this work prevents us from doing automatic grafting later on. As part of the work which will follow the automating grafting, we would have the need to distinguish between statically linked code, and dynamically linked code. To do so, we would have to make the fix-point produce 2 outputs, and make each package select from the static-linked fix-point or from the dynamically-linked fix-point, as part of their build inputs. Note that in non-grafting mode, both fix-points would be identical, and in the grafting mode, the dynamically-linked fix-point would be peeled off to include the automatic grafting. This implies that we still explictly use
This is done by doing a late binding of the fix-point of the language packages. Currently haskell-packages, emacs-packages, python-packages are all doing a fix-point binding before allowing any override to happen. With SOS and the double-bracket proposal, the fix-point would be bound to its result at the time of Nixpkgs fix-point. Thus all the copy and extensions would be handled as long as they are tracked. |
Closing all draft RFCs. Please feel free to reopen when this is ready for general community review and the RFC shepherd process. |
This pull request has been mentioned on Nix community. There might be relevant details there: https://discourse.nixos.org/t/in-overlays-when-to-use-self-vs-super/2968/5 |
1 similar comment
This pull request has been mentioned on Nix community. There might be relevant details there: https://discourse.nixos.org/t/in-overlays-when-to-use-self-vs-super/2968/5 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2022-10-19-nixpkgs-architecture-team-meeting-14/22578/3 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2023-03-06-nixpkgs-architecture-team-meeting-31/26070/1 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/working-group-member-search-module-system-for-packages/26574/1 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2023-04-03-nixpkgs-architecture-team-meeting-35/26950/1 |
Motivation
Making Nix packages more declarative, and without the usual
stdenv.mkDerivation
, norcallPackage
functions provide multiple benefits such as:override
andoverrideDerivation
functions by the update (//
) operator.nix-env
tool.stdend.mkDerivation
andcallPackage
to be garbage collected by the Nix interpreter./cc @peti
Rendered