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

Single package fixpoint #296769

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

roberth
Copy link
Member

@roberth roberth commented Mar 17, 2024

Description of changes

This provides an alternative to mkDerivation - a different interface which solves a significant number of problems.

TODO

  • Port this fileset layer. It will be nicer, not having to "unset" things for derivation.
  • ...?

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Creates objects that have overlay-based private attrs in their closure.

(cherry picked from commit 0067cf4)
@github-actions github-actions bot added 6.topic: stdenv Standard environment 6.topic: lib The Nixpkgs function library labels Mar 17, 2024
This shows the `deps` pattern, merging the following into a
single fixpoint, from NixOS#273815

 - package function args (`this.deps`)
 - mkDerivation args (`this.setup`)
 - derivation args (`this.drvAttrs`)
 - package attributes (`this.public`)
Demo:

```diff
diff --git a/pkgs/by-name/he/hello/package.nix b/pkgs/by-name/he/hello/package.nix
index 0cf0ae493b72..69921e14ffc6 100644
--- a/pkgs/by-name/he/hello/package.nix
+++ b/pkgs/by-name/he/hello/package.nix
@@ -1,9 +1,13 @@
-{ mkPackageWithDeps, layers }: mkPackageWithDeps ({ stdenv, fetchurl, testers, lib, callPackage, nixos }: [
+{ mkPackageWithDeps, layers }: mkPackageWithDeps ({ stdenv, fetchurl, testers, lib, callPackage, nixos, haskellPackages, omnidoc }: [
   (layers.derivation { inherit stdenv; })
   (this: old: {
     name = "hello";
     version = "2.12.1";

+    deps = old.deps // {
+      omnidoc = haskellPackages.pandoc;
+    };
+
     setup = old.setup or {} // {
       src = fetchurl {
         url = "mirror://gnu/hello/hello-${this.version}.tar.gz";
@@ -24,6 +28,10 @@
       run = callPackage ./test.nix { hello = this.public; };
     };

+    public = old.public // {  # passthru, just to show that it works
+      inherit omnidoc;
+    };
+
     meta = with lib; {
       description = "A program that produces a familiar, friendly greeting";
       longDescription = ''
```
@Ericson2314
Copy link
Member

So continuing from NixOS/nix#10610 (comment), my thought is similar to the talk of NixOS module system modularity. Just as we'd like multiple PostgreSQL "instances", for example, I think we should be very careful to destinguish a package in isolation, which does not care how it is used (e.g. choosing its deps) --- it is function, from an instantiate package in a package set, which is not a function.

@roberth
Copy link
Member Author

roberth commented Apr 27, 2024

I think we should be very careful to destinguish a package in isolation, which does not care how it is used (e.g. choosing its deps)

I agree. For this it is important that the package presents itself as a function with a stable interface.

Suppose you're packaging app, which is a function of { stdenv, postgresql }, but it doesn't work with the default postgresql version in Nixpkgs.
In the current architecture you have two options:

  1. make it a function of { stdenv, postgresql_12 }
  2. set a default value, callPackage ../by-name/ap/app/package.nix { postgresql = postgresql_14; }.

In this situation, (1) is wrong because it chooses its deps, and it's not a stable function this way, because callers need to be aware of the current default version.
However (2) is not desirable; contributors don't want to touch all-packages.nix, if they even believe they're allowed to.

With this implementation of the single fixpoint idea, they can preserve the existing function interface, and set a default in the same file - without touching all-packages.nix:

package.nix:

{ mkPackageWithDeps, layers }:
mkPackageWithDeps ({ pkgs, stdenv, postgresql, ... }: [
  (layers.derivation { inherit stdenv; })
  (this: old: {
    name = "app";
    deps = old.deps // {
      postgresql = pkgs.postgresql_14;
    };
    setup.buildInputs = [ postgresql ];
  }
])

deps.postgresql has a default, but its value can still be set with callPackage ./package.nix { postgresql = ...; } or .override.
It is still a proper function that way, and its signature is stable.

@Ericson2314
Copy link
Member

Ericson2314 commented Apr 27, 2024

But what happens if I try to instantiate this package within a package set that doesn't have a postgresql_14 attribute?

I would prefer something like either

  • a separate package.nix and deps.nix
  • a package.nix with
    {
       function = ...;
       dep-defaults = ...;
    }

for a bit more of a by-construction guarantee the package function stands alone without the defaults.

, testers
, hello
}:
{ mkPackageWithDeps, layers }: mkPackageWithDeps ({ stdenv, fetchurl, testers, lib, callPackage, nixos }: [
Copy link
Member Author

Choose a reason for hiding this comment

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

Consider adding layers as well as a function overrideLayers to the call here, such that they don't come from the normal fixpoint. Otherwise it's not possible to override that layers, which someone will inevitably need.

It also makes the separation of concerns a bit nicer:

# 1 parameter to declare that we'll be doing it the layered way
{ mkPackageWithDeps }: mkPackageWithDeps ({
  # all the rest goes here
  layers, ...
}: [
  # ...
])

That changes the fixpoint "state" from just overlays to { overlays = <composition of functions>; layers = <attrset of functions brought into scope by mkPackageWithDeps>; }, but that seems manageable and not something users have to be aware of, as long as it works correctly. It's still just a single fixpoint, so it seems feasible to get it right.

@roberth
Copy link
Member Author

roberth commented Apr 27, 2024

  • a package.nix with
    nix { function = ...; dep-defaults = ...; }

This could be implemented as syntax sugar. The package.nix file can choose how to construct itself:

{ mkPackageWithDefaults }: mkPackageWithDefaults {
  function = ...;
  dep-defaults = ...;
}

If we don't implement it as syntax sugar, the "obvious" code would not have a single fixpoint, which would instantly make overriding incoherent. Make it syntax sugar, and we avoid that problem.

@Ericson2314
Copy link
Member

The package.nix file can choose how to construct itself

Ah, but this is just what I am taking issue with, I want to force packages to not mix together the package function and the default argument choosing. I agree making people edit all-packages.nix is bad, but I think the separation between choosing deps and declaring dep parameters is very good.

If we think packages are "straight away" put in the big fixed point, this can all seem academic, but if we think we'll first build a library of package functions, and then build the big fixed point from that, the distinction gets more practice.

The "library of package functions" (recipe book) is very important to me as the only thing which is truly modular.

@roberth
Copy link
Member Author

roberth commented Apr 27, 2024

Ah, but this is just what I am taking issue with, I want to force packages to not mix together the package function and the default argument choosing.

Sure, but then you're not compatible with by-name/*/*/package.nix. I'd be ok with enforcing something, such as by loading a different file that only has the expression you suggested, but that seems like a constraint that's easy to add later on.

if we think we'll first build a library of package functions

That sounds a lot like what was done in the past and what has got us into the current situation, unless we have the discipline not to add unnecessary fixpoints in the process, as you suggest.

I am a bit skeptical though:

  • I doubt that a huge bag of functions is a pleasant to use intermediate state of affairs.
  • Without any conventions emanating from the single fixpoint, we'd probably end up with functions that wrap other functions, such as the language-specific builders. Even if those don't have their own fixpoint logic, they're going to need a degree of rewriting to fit the those conventions, once we do establish them in the end.

@Ericson2314
Copy link
Member

That sounds a lot like what was done in the past and what has got us into the current situation, unless we have the discipline not to add unnecessary fixpoints in the process, as you suggest.

Yeah I want to do one round of importing all the functions and calling nothing, maximally flexible. No fixed points, no overriding, non of that. Just library of recipes, everything is in "fully functored form" as the OCaml people would say.

And then I want to do another round of one big fixed point, putting everything together.

Without any conventions emanating from the single fixpoint, we'd probably end up with functions that wrap other functions, such as the language-specific builders.

You are contrasting that the layers thing you have? I haven't thought about that so much but I think there is no conflict.

@roberth
Copy link
Member Author

roberth commented Apr 28, 2024

@Ericson2314 I think we agree on the overall strategy.

@Atemu
Copy link
Member

Atemu commented May 1, 2024

Yeah I want to do one round of importing all the functions and calling nothing, maximally flexible. No fixed points, no overriding, non of that.

I particularly like this idea because it could make it possible to "override" elements of the package set that are not drvs or attrsets in their "final form" (where a .override attr could be added) such as builders or wrappers.

@purepani
Copy link
Contributor

purepani commented May 8, 2024

@roberth This looks really cool! I was wondering if you had mocked up a use of this type of function in a similar manner to dream2nix(since you mentioned it in the issue); I was thinking of trying to do that both as a learning opportunity and to see if I could figure out dynamic derivations as well, but I also didn't want to redo anything you were working on, especially since these things would probably be pretty hard for me given that all of this is pretty new.

Also wanted to ask @DavHau since you also might be working on something like this, or might have some insight.

@roberth
Copy link
Member Author

roberth commented May 17, 2024

@purepani, that sounds like a lot to do simultaneously. My goal was to show that it can be done, and establish a starting point. Haven't been able to spend much time on it since. I'd be happy to explain things if you want to try it.

@nbp
Copy link
Member

nbp commented May 18, 2024

While I definitely agree and I am pleased with the result. I would have one recommendation which is to make it at first such that the existing function can still be used without making any changes, and then progressively replace one by the other.

The idea would be to avoid having any down-time as these kind of changes would likely takes months / years before they are complete, and having something backward compatible would also help with the migration process.

Thus callPackage should become a function which create a set with all the inputs and the function the be called; stdenv.mkDerivation should be a function which extends the result of callPackage with the drvOut public attributes.

@roberth
Copy link
Member Author

roberth commented May 20, 2024

@nbp Not sure I understand what you mean by downtime. This PR adds an alternative to mkDerivation, which packages can migrate to on an individual basis. The returned / public attributes are largely compatible with the existing packages (except being more intentional / "cleaner", which should be fine, and if not, we could have a layer that adds back a bunch of bloat).

Thus callPackage should become a function which create a set with all the inputs and the function the be called

That sounds rather invasive. callPackage is widely used for

  • producing packages
  • performing dependency injection

I don't see how returning functions and arguments helps with those expectations or reducing the number of fixpoints per package. Rather it hints at adding another one.
My very intentional intent behind the change to lib.customisation.callPackageWith is to remove .override from its responsibilities when the package definition is a single-fixpoint package. That way it can be handled simply and correctly by the package fixpoint.
If we were to make callPackage return result // { inherit origArgs function; }, we make available exactly the info that allows .override to be reconstructed and re-added to the package. That works against the goal of forcing all package-based overrides to go through a single fixpoint. And if we don't go through a single fixpoint, we'll end up recreating those same composition / correctness issues this PR is supposed to solve.

@roberth
Copy link
Member Author

roberth commented May 20, 2024

I think you're applying a solution you've thought of for S.O.S. here, but prematurely. My understanding of that concept is that it operates at the package set level, unlike this PR.
Once packages consist of a single fixpoint, it will be easy to "open them up" and let them be specified as a single function instead of a file that also ties the knot. I can (vaguely) see how that could then enable simplifications at the package set level.

So I'd be happy to replace callPackage at that point, but until then, this has to work with the existing large-scale infrastructure of Nixpkgs, and since RFC 140, that basically also means hooking into callPackage somehow. If it wasn't for that, I'd be happy to sidestep callPackage, and avoid the { mkPackageWithDeps, layers }: mkPackageWithDeps opening. The purpose of that is just to integrate with Nixpkgs how it works today, selecting a package producing helper just like packages now pick stdenv.mkDerivation or a language-specific one. When the single fixpoint approach is adopted, this decision is reduced to a selection of layers instead of a choice of function, and we'll be able to remove all those openings and let the packages just be a function for the single package fixpoint.

@roberth
Copy link
Member Author

roberth commented May 20, 2024

Maybe what you're asking for is a way to skip ahead to that point where we can change all package files to be single-fixpoint style?
I think we could come up with a layer function that basically just calls the existing mkDerivation, language-specific functions, etc, so that all package files can migrate mechanically without much of the benefits of actually having a single fixpoint.
When that's done, we remove those openings and something like S.O.S. could fix (EDIT: probably, encapsulate) those functions as it pleases.

@purepani
Copy link
Contributor

@purepani, that sounds like a lot to do simultaneously. My goal was to show that it can be done, and establish a starting point. Haven't been able to spend much time on it since. I'd be happy to explain things if you want to try it.

I'll definitely let you know if I have any questions once I have a chance to dig into it!

@purepani
Copy link
Contributor

purepani commented Jun 1, 2024

Having some time to think about this, I'm wondering if it would make sense to have some sort of "typing information" file(like header files in C++), and sort of introduce gradual typing into this.
That's probably beyond the scope of this idea, but I'm only wondering that because it might make the whole merging speed issue faster, if we can, say, write a typing file for a package that defines an interface(as mentioned earlier), of what the outputs of it are(in terms of typing) and the inputs are.
I'm imagining taking, for example, the already existing package.nix file and writing a typing.nix file that looks like this

{lib, postgres}:
inputs = {
   postgres=postgress.type
};
outputs={
out=lib.type.out;
fetch-deps=lib.type.fetchdeps;
};

Then you could prerun, and cache the typing information, and then use that information to help in any sort of merging process, only recompiling when any header files change.

This feels like something that could be more useful to implement on a language level, but I thought I'd put it out there since it's an extension of what was being talked about earlier.

@roberth
Copy link
Member Author

roberth commented Jun 9, 2024

merging speed issue faster

Something like this is probably half way in terms of performance between the module system and hand-coded merging (as we do with overrideAttrs etc).

We could go faster by removing the priorities, or slower by always checking the value. To really go faster though, I think we need a builtins.zipAttrsWith' that doesn't perform a Nix function call when there's only a single value.

To bring this closer, we could make a gradual change to encapsulate. Instead of
{ extend = f: encapsulate (extends f layerZero); } in encapsulate, we could do something more general, perhaps turning the hardcoded lib.extends into a parameter; { extend = x: encapsulate extender (extender layerZero x); }, where x could be lib.extends or something that helps with merging. Or { extend = x: encapsulate ((fixed.extender or extends) layerZero x); }, adding no memory cost in the default case where it's just extends.
A potential limitation of this approach is that it potentially requires reassembling the typing metadata for every instance, while it would be preferable to only assemble, say, an attrset of merge functions only once per language specific build helper (ie one of the generic-builder.nix-es).

Meanwhile @nbp is working on a proposal (in his free time, afaik) to make deep overrides easier to write, which might also run faster in the evaluator just by virtue of being interpreted more directly instead of a bunch of function calls with all the Nix function bells and whistles. Idk, maybe we should get someone from SpiderMonkey on Nix ;)

@roberth
Copy link
Member Author

roberth commented Jun 17, 2024

I think the next step is to try and convert an existing generic-builder.nix to use this, without trying to change the data model - ie preserve all the nasty removeAttrs and // operations on the recipe function (the "user"-provided function), so that the packaging logic is 1:1 compatible, or at least very close.

  • Feasible to actually migrate
  • No "improved" data model for users (but a path towards that maybe?)
  • Can fix overriding composition complexity and bugs

Steps

  1. Extract functions from generic-builder.nix that perform the attribute manipulation, not including calls to .override* or lib.customisation. (as previously suggested by @Ericson2314)
  2. Create a new function that calls lib.encapsulate and uses those functions to achieve the same wiring.
  3. Try instantiating the applicable packages with it, see if anything breaks.
  4. (Optional) Create a variation of the function that also includes the layer with deps to replace callPackage and fix the override/overrideAttrs interaction.
    This will require package to make a change at the top of the file, e.g.
    { mkLangDerivation, foo, bar }: mkLangDerivation { ... }
    -> { mkLangDerivationWithDeps }: mkLangDerivationWithDeps ({ foo, bar }: { ... })

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict 6.topic: lib The Nixpkgs function library 6.topic: stdenv Standard environment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants