Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

nbp
Copy link
Member

@nbp nbp commented Feb 26, 2017

Motivation

Making Nix packages more declarative, and without the usual stdenv.mkDerivation, nor
callPackage functions provide multiple benefits such as:

  • Replace the override and overrideDerivation functions by the update (//) operator.
  • Make faster lookup for packages names, as done by the nix-env tool.
  • Remove the unused memory held by stdend.mkDerivation and callPackage to be garbage collected by the Nix interpreter.
  • Normalize inputs of packages, to be used for future grafting techniques.

/cc @peti

Rendered

@bennofs
Copy link

bennofs commented Feb 27, 2017

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 qux, paying attention to memory usage:
(I hope this is how nix actually evaluates, not having read the nix evaluator code myself)

old-style

First, we evaluate the callPackage for qux. This results in an attrset containing references to thunks for bar and foo. Next, we need to evaluate the mkDerivation, which forces bar and foo, where each of those requires another attrset (with foo as entry for bar and no entries for foo). Since qux refers to them both, none of these can be garbage collected, so in total, we allocate 3 attrsets with 0, 1 and 2 entries.

After that, for building, nix will take the out attribute of the set, now allowing all these attrsets to be freed. But this only happens at the very end.

Note that foo is only evaluated once, because there's a single thunk representing the derivation for it. But all these thunks keep the callPackage-induced attrset alive.

new-style

First, we evaluate self (this is not particular interesting, just some knot-tying). Then, we evaluate the qux attribute of that set, forcing the asDerivation call for qux. This forces buildDeps to be computed, and since buildDeps is part of the package attribute set, and that set is a part of self (or s in the code), this buildDeps attrset will not be freed until the end of the evaluation, when self is freed.

Next, we need to asDerivation the deps, foo and bar. Because there is no top-level thing that caches those asDerivation calls, we also need to call asDerivation foo again when we compute bar. But each of those calls will force the buildDeps for the respective packages, which are cached (since they are part of self.<name of package>). So, all in all, we still have 3 attrsets with 0, 1 and 2 entries, but we spend more time since we don't cache asDerivation.

Am I overlooking something? Do those memory problems only occur when you actually use overrides?

@zimbatm
Copy link
Member

zimbatm commented Mar 1, 2017

@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.

@bennofs
Copy link

bennofs commented Mar 1, 2017

@zimbatm that makes some more sense.

Say we do the following:

  bar2 = callPackage ({ foo }: deriv "bar" [ foo ]) { inherit foo; };
  qux2 = qux.override (original: original // { # my .override implemention in the gist has a bug, should be `args // f args` instead of just `f args`
    bar = bar2;
  });

Then, when evaluating qux2, we need to evaluate args, which first produces the original attrset and then applies the override function to it. However, at this point, after the override function's result has been forced, there shouldn't be anything left that's holding onto original, no? We need to make sure that we fully force the new args, or else there might be attributes of the new args that still refer to the old arguments and are not evaluated yet, thus holding onto original. Is that the issue here? But then I don't see how the new approach would fix this, as you can still construct a situation where some of the attributes depend on the old attrset, thus holding onto it the new args aren't deep-forced.

@zimbatm
Copy link
Member

zimbatm commented Mar 2, 2017

@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.

@layus
Copy link
Member

layus commented Mar 19, 2017

Some packages have a generic argument, that needs to be passed when calling callPackage.
For example, some packages have a qt argument, which can be either qt4 or qt5.

How would you handle that ?

@Ericson2314
Copy link
Member

@nbp We should finally get around to figuring the out cross and bootstrapping parts of this plan :).

@Profpatsch
Copy link
Member

Profpatsch commented Apr 4, 2017

@nbp It looks to me like the actual build plan is missing from the specification of packages; how would I specify that haskellPackages use a different build scheme than stdenv packages?

@zimbatm zimbatm changed the title SOS: Simple Override Strategy. [RFC 003] SOS: Simple Override Strategy. Apr 4, 2017
@zimbatm zimbatm changed the title [RFC 003] SOS: Simple Override Strategy. [RFC 0003] SOS: Simple Override Strategy. Apr 13, 2017
@nbp
Copy link
Member Author

nbp commented Jun 25, 2017

@bennofs

Am I overlooking something? Do those memory problems only occur when you actually use overrides?

Mostly yes. The callPackage & mkDerivation do a bunch on attribute set instantiation to recall how to make a call to the original function. Keeping this information around has a small cost, but it is multiplied by each package that you use.

This proposal will move the override part, such that it is done before the calls to mkDerivation, and with no need for callPackage any more. Thus, we would still have the mkDerivation but we would not need to keep around attributes which are dedicated to the reflexivity of the derivations.

This will also definitely help nix-env command for listing the set of package names which are available, as finding the names would no longer require doing part of the work dedicated to instantiate a derivation.

@zimbatm

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.

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
Test: nix-instantiate ./ -A --eval-only --strict --json | sort | uniq -c

      1 "/nix/store/bl48w7h24n6379fa200yv60n4pwynd9c-stdenv"
     42 trace: callPackageWith
      1 trace: __functor
      8 trace: override(attrs)
 
      1 "/nix/store/zpd3d0wypl6lwcsqg9a8cig26cg06h6v-feh-2.18.2"
    126 trace: callPackageWith
     10 trace: __functor
     14 trace: override(attrs)
      7 trace: overrideDerivation
 
      1 "/nix/store/arkzhm2wpyykl86dcg5k2z7rnnln6pw2-firefox-54.0"
    300 trace: callPackageWith
     14 trace: __functor
     21 trace: override(attrs)
     17 trace: overrideDerivation
 
      1 "/nix/store/4msrf94gvf1xx3d06ppcwv58ha07pqgz-kate-17.04.2"
    893 trace: callPackageWith
     46 trace: __functor
     21 trace: override(attrs)
     20 trace: overrideDerivation

@nbp
Copy link
Member Author

nbp commented Jun 25, 2017

@layus:

For example, some packages have a qt argument, which can be either qt4 or qt5.

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 /</ is some-kind of update operator which recursively update until the />/ operator is met. If you have better suggestions, feel free to suggest them.

Basically, this is as-if we had rewritten the full set, but only changed the qt attribute from the buildDeps attribute set. At the end, when the derivation would be evaluated, the buildDeps attribute set would be given as argument to the buildInputs function, which would take the latest value of the qt argument.

[1] https://github.com/NixOS/nixpkgs/blob/a93225fc6c714b3035bb81e449a8365bec81d7ae/pkgs/top-level/all-packages.nix#L9683-L9684

@nbp
Copy link
Member Author

nbp commented Jun 25, 2017

@Profpatsch

how would I specify that haskellPackages use a different build scheme than stdenv packages?

The idea is that each expression must provide a drvBuilder, which is a way to view the current attribute set as a derivation. Thus haskellPackages, pythonPackages would basically provide the similar function like before.

The top-level of Nixpkgs is in charge of calling the drvBuilder on all the packages, and each package would be in charge of doing the same to its buildDep. Another option would be for Nixpkgs to call the drvBuilder on all packages before providing the self argument, if the derivation-view is request, and not a name / license view. This second approach might actually be more efficient.

@Profpatsch
Copy link
Member

The idea is that each expression must provide a drvBuilder, which is a way to view the current attribute set as a derivation. Thus haskellPackages, pythonPackages would basically provide the similar function like before.

That’s awesome, yes. Build instructions instead of actually building at the lowest level, as e.g. in Haskell’s IO data type.

@Ericson2314
Copy link
Member

Ericson2314 commented Jun 26, 2017

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.

'';
};
}
```
Copy link
Member

@zimbatm zimbatm Jun 30, 2017

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?

Copy link
Member

@layus layus Jul 2, 2017

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; }.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zimbatm

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.

@zimbatm
Copy link
Member

zimbatm commented Jun 30, 2017

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.

@0xABAB
Copy link

0xABAB commented Jun 30, 2017

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.

@zimbatm
Copy link
Member

zimbatm commented Jul 2, 2017

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.

@Ericson2314
Copy link
Member

Incremental should work fine.

@nbp
Copy link
Member Author

nbp commented Jul 2, 2017

@zimbatm

Is it necessary to switch all of nixpkgs at the same time or could a proof-of-concept be implemented for a subset?

This is part of the current proposal.
We can do it incrementally, if we add to Nixpkgs the way to run the drvBuilder for all these attribute sets.

@layus
Copy link
Member

layus commented Jul 2, 2017

Maybe not an urgent concern, but I have always been bothered by the two ways to write attrsets in nix:
{ a = { b = "c"; }; } == { a.b = "c"; }. We could instrument nix to keep in mind the way attrsets were declared.

A special builtin (say /<>/) could use that information when making deep overrides, like so

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

@nbp
Copy link
Member Author

nbp commented Jul 2, 2017

@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.

@nbp
Copy link
Member Author

nbp commented Jul 2, 2017

@layus

We could instrument nix to keep in mind the way attrsets were declared.

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

@layus
Copy link
Member

layus commented Jul 2, 2017

Another question: some drvBuilder's like buildPythonPackage do change the name of the derivation. In case of python, they add a prefix which defaults to python.libPrefix + "-", which is why all the python packages are named pythonX.Y-packageZ. To reproduce this, you would need to evaluate the drvBuilder even for the name view. Would that still be more efficient than the current situation ?

@nbp
Copy link
Member Author

nbp commented Jul 2, 2017

@Ericson2314

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.

@zimbatm

but will be harder to understand one overrides or conditionals are added to the mix.

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}";
  };
}

@0xABAB
Copy link

0xABAB commented Jul 2, 2017

@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.

@nbp
Copy link
Member Author

nbp commented Jul 2, 2017

@layus

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.

@nbp
Copy link
Member Author

nbp commented Jul 2, 2017

@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 pkgs.path as this is the same as <nixpkgs>, and you already have super within the overlay:

let
  newpkgs = import <nixpkgs> {
    overlays = [ (self: super:  super /<>/ { python27.numpy = ...; } ) ];
  };
in newpkgs.inkscape

@Profpatsch
Copy link
Member

Profpatsch commented Jul 2, 2017

If we compare this RFC with the one about encryption, then it's clear there is a "demand" for it.

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.

@Ericson2314
Copy link
Member

@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?

@edolstra
Copy link
Member

edolstra commented Mar 1, 2018

@nbp I wrote a document with Nix language ideas: https://gist.github.com/edolstra/29ce9d8ea399b703a7023073b0dbc00d. It has quite a bit of overlap with SOS.

@Ericson2314
Copy link
Member

Ericson2314 commented Mar 1, 2018

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 foo = f super.foo; for arbitrary f.

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!

@nbp
Copy link
Member Author

nbp commented Mar 3, 2018

@edolstra , really nice proposal, and much better syntax. I have multiple comments:

  • builtins.fix should not be the only way. If we want to experiment futher than what we have today, we want to be able to provide another argument to this extensible attribute set. One example of such thing is the security-update branch which needs to call the Nixpkgs twice. Having a builtin to call it with something else, wether it is it-self or not would be desirable.

  • Do we need "super"? Yes we do, not always, and not necessary at the top-level. This problem appeared multiple times in the module system already, as how: "how do we remove a definition from a specific module." The current solution for that is not great, as it involves excluding the module to redefine all the options your-self.

  • super (ordering) is madatory to ensure that a patched input is not patched after it is being renamed:

    let
      overlay1 = {{ hello = package {{ }}; guiHello = package {{ inherit hello; }}; }};
      overlay2 = {{ hello_fork = hello; guiHello = package {{ hello override = hello_fork; }} }};
      overlay3 = {{ hello = package {{ name override = "goodbye"; }}; }};
    in
      import <nixpkgs> { overlays = [ overlay1 overlay2 overlay3 ]; }
    

    This issue might not seems significant with Hello but did appear previously with Haskell packages. We do not always want the final result, nor do we always want the first result.

  • Overriding dependencies. This issue is solved in SOS, by introducing the strictly dynamically scoped names. The problem that you got is that to what is bound a name within an extensible attribute set, if the attribute is not yet defined.

    let x = 4; in
      (builtins.fix ( {{ y = x; }} // {{ x = 2; }} )).y
    

    If you think of NixOS, then you would expect everything to be provided by the fix-point, thus having a dynamic name binding.

    {{
      hello.args = {{
        // looking for `make` within args, then within hello, 
        // then within the extensible attribute set which
        // contains hello, then out-side of it.
        buildPhase = "${make}";
      }}
    }}
    

    I think such lose bindings should be avoided, in order to avoid future nightmares. I would much prefer to have some implicit arguments for buildPhase and have clear bindings to one extensible attribute set which contains all inputs, such as:

    {{
      hello.args = {{
        inputs = {{ inherit make; }};
        // binds all free variables to attributes within
        // inputs. (once the packages is being realized)
        buildPhase = strict with .inputs; "${make}";
      }}
    }}
    

    Yes, this implies repeating the list of inputs, but I think this is better to have this repeated because it and easy to understand and override than having lose bindings (see below).

    I will note that this can be quite handy too, for doing override of some inputs without creating a new name:

    {{
      gecko.args.inputs.cc = {{
        name = ...;
        src = ...;
      }}
    }}
    
  • Everything should be planned ahead as being extensible, because a builtins.fix call is required, otherwise the same extension problem re-appears, as we have currently.

  • The lookup of attribute would be a nightmare both for performance and sanity reasons, every double-brakets introduce a scope which might be defining the attribute that we are currently looking at.

    {{ a = {{ b = {{ c = {{ d = {{ e = x; }}; }}; }}; }}; }} // import ./x.nix
    
  • For both SOS and the double-brackets proposal, there is no need for any builder arguments if we have a way to override anything freely. The way to define a package then might just be an extension of the builder.

    {{
      unix = {{
        name :: {
          description = "Name of the package";
          type = types.string;
        }
        version :: {
          description = "Version number of the package"
          type = types.string;
        }
      }};
      defaultBuilder = unix;
      hello = defaultBuilder // {{
        name = "hello";
        version = "3.14";
      }};
    }}
    

    This approach of extending the builder is better than naming it, because making a package is now identical to overriding a package. Doing so should be favored, independently of the approach taken, as this implies that anybody who can change some packages sources, name, inputs or configuration flags, will now be fully competent for contributing to Nixpkgs, which makes it a better learning curve.

  • I think the SOS way of doing recursive updates/zipping might be nicer with the double-brakets notation, where the double-bracket are used to mention that we should update/zip recursively with the left-hand-side.

    ( { x = { a = 1; }; y = { c = 1; }; }
    // {{ x = //@previous: { b = previous.a; }; y = { d = 2; }; }}
    ) == {
      x = { b = 1; };
      y = { c = 1; d = 2; }; # recursive update.
    }
    

    Also note: SOS requires the update operator ( // ) to work for functions too, to make it simple to solve the haskell package issues. This should not be an issue as today this is an error.

  • Both SOS and the double-bracket proposal solves the haskell packages issue as well as other languages packages fix-points, but to do so, we either need a safe way to recursively decent in Nixpkgs attribute set, or to enumerate the list of attributes where it is safe to go down.

  • the attribute annotation is really nice, and I think this could be created independently of the rest of these proposals (I know that you rely on the merge-ability to solve the extensibility issue in fix-points), but this could be something simpler to add, such as builtins.checkTypes, which ensure that all attributes within an attribute set are validated with the annotated types.

    {
      package = {
        name :: {
          description = "Name of the package";
          type = types.string;
        }
      };
    }
    

    I will note, that we will face the same strictness issue as faced in the NixOS module system, if the recursive decent for checking types is implicit, and not explicit.

    let
      implicit = {
        bar = {
          name :: { type = types.string; } = 42;
        };
      };
      explicit = {
        bar :: { type = types.attrs; } = {
          name :: { type = types.string; } = 42;
        };
      };
    in
      builtins.checkTypes implicit == true && # if an explicit decent is implemented
      builtins.checkTypes explicit == false
    
  • All the extra keywords of the double-brackets proposal could later be implemented as sugar on top of the attribute annotations.

    {{
      hello = package {{ ... }};
    }}
    

    could be replaced by:

    {{
      hello :: { package = true; } = {{ ... }};
    }}
    

@shlevy
Copy link
Member

shlevy commented Mar 3, 2018

(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 extensible-records.nix, this is not intended to be a permanent solution, merely a way we can validate and iterate on the design and semantics without committing to language changes up-front.

@shlevy
Copy link
Member

shlevy commented Mar 3, 2018

@Ericson2314 ^ FYI

@shlevy
Copy link
Member

shlevy commented Mar 3, 2018

@nbp responding to some of your comments in light of my library impl and my idea about assignment semantics.

  • builtins.fix should not be the only way.

I think to pass different arguments you can just use with. e.g. safe-foo = with securePkgs. But maybe a specific example would be helpful

  • Do we need "super"?

My answer to this is that you can do foo := <expr> and in expr foo would refer to super.foo.

  • super (ordering) is madatory to ensure that a patched input is not patched after it is being renamed:

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 with defining hello_fork pegs hello to super.hello, but the with defining guiHello does not because super is only used for the attribute you're currently assigning.

  • Everything should be planned ahead as being extensible, because a builtins.fix call is required, otherwise the same extension problem re-appears, as we have currently.

Agreed.

  • The lookup of attribute would be a nightmare both for performance and sanity reasons, every double-brakets introduce a scope which might be defining the attribute that we are currently looking at.

Yes, I think Eelco explicitly forbade this.

  • For both SOS and the double-brackets proposal, there is no need for any builder arguments if we have a way to override anything freely. The way to define a package then might just be an extension of the builder.

Agreed, though maybe it would be good then to have a version of // that doesn't allow adding any attributes.

  • Both SOS and the double-bracket proposal solves the haskell packages issue as well as other languages packages fix-points, but to do so, we either need a safe way to recursively decent in Nixpkgs attribute set, or to enumerate the list of attributes where it is safe to go down.

I'm not completely sure I understand what you mean here, but I think this issolved by recursing only into {{}} sets.

@nbp
Copy link
Member Author

nbp commented Mar 3, 2018

I think to pass different arguments you can just use with. e.g. safe-foo = with securePkgs. But maybe a specific example would be helpful

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 self (and later self.static, self.dyn with self being an alias of self.static) for finding the dependencies. If we rename self to anything else, we should at least ensure that we would have this possibility.

  • Both SOS and the double-bracket proposal solves the haskell packages issue as well as other languages packages fix-points, but to do so, we either need a safe way to recursively decent in Nixpkgs attribute set, or to enumerate the list of attributes where it is safe to go down.

I'm not completely sure I understand what you mean here, but I think this issolved by recursing only into {{}} sets.

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.

@shlevy
Copy link
Member

shlevy commented Jan 17, 2019

Closing all draft RFCs. Please feel free to reopen when this is ready for general community review and the RFC shepherd process.

@nixos-discourse
Copy link

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
@nixos-discourse
Copy link

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

@nixos-discourse
Copy link

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

@nixos-discourse
Copy link

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

@nixos-discourse
Copy link

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

@nixos-discourse
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants