change mechanism for `deps`; add `depsRequired`; add `depsFrom`. #10

DavHau · 2022-12-22T12:35:19Z

@roberth It would be amazing if you could review this. I really want to get this right.

With this commit, the user is forced to explicitly specify all required dependency names via depsRequired. options.deps contains generated options for all names of depsRequired. These options are populated with defaults from depsFrom, The examples under ./examples show how this is used.

The coercedTo type of deps had some weaknesses:

It was not possible to specify required dependencies explicitly, because it was not possible to create nested options likeoptions.deps.bash = mkOption ....
Because of (1.) I had to add a manual check in makeModule.nix to ensure that all required deps are populated by the user.

Having actual options for all required dependencies allows to reduce complexity and hand the checking off to the module system.

Also, when rendering the options for a package to a doc, all dependencies now appear as an option, which I think is great. It allows the user to see all inputs of a package without having to read the code.

This whole deps mechanism is generic enough to suit as a general inputs. It probably should be renamed to inputs in the future, but I don't want to clutter this PR too much.

EDIT:
options.stdenv can now probably be moved to options.deps.stdenv. It is basically just a dependency of the derivation and I see no need for special treatment.

With this commit, the user is forced to explicitly specify all required dependency names via `depsRequired`. `options.deps` contains generated options for all names of `depsRequired`. These options are populated with defaults from `depsFrom`, The examples under `./examples` show how this is used. The `coercedTo` type of `deps` had some weaknesses: 1. It was not possible to specify required dependencies explicitly, because it was not possible to create nested options like`options.deps.bash = mkOption ...`. 2. Because of (1.) I had to add a manual check in makeModule.nix to ensure that all required `deps` are populated by the user. Having actual options for all required dependencies allows to reduce complexity and hand the `checking` off to the module system. Also, when rendering the options for a package to a doc, all dependencies now appear as an option, which I think is great. It allows the user to see all inputs of a package without having to read the code.

roberth

I don't like killing the deps pattern. I think it does a pretty good job at steering towards a separation of concerns, steering package authors to a situation where all deps are overridable, potentially even improving on callPackage in this regard.

Of course it can be defeated, but package authors who never see pkgs outside of deps = { pkgs, ... }: will not think of referencing it directly. I'm also surprised to see that pkgs is even in the package module arguments. Was that needed for something? (before this PR)

roberth · 2022-12-22T13:00:23Z

examples/flake-parts/htop/htop.nix

-        systemd
-        ;
+    # This must be complete, otherwise options would be missing from `deps`.
+    depsRequired = {


This rename seems like a step back.

roberth · 2022-12-22T13:02:32Z

modules/derivation-common/interface.nix

-        callDeps
-        (t.lazyAttrsOf t.raw);
+    # Unspecified `deps` are taken from `depsFrom`. It's a source for defaults.
+    depsFrom = l.mkOption {


Seems like it should be this then

Suggested change

depsFrom = l.mkOption {

defaultDeps = l.mkOption {

or depsDefaults

roberth · 2022-12-22T13:03:54Z

modules/drv-parts.nix

-                  ;
+              # Define dependencies from the "outer world" only via `depsFrom`.
+              # This allows for easy overriding via `config.deps` later.
+              depsFrom = {inherit (pkgs) fetchurl python};


Where does this pkgs come from?
The point of having a function here is to avoid bringing pkgs into the larger scope and seducing people into using it directly.

This example is bad. depsFrom should never be specified from within the package module itself. It should be set from outside.

roberth · 2022-12-22T13:14:19Z

modules/derivation-common/interface.nix

+    deps =
+      l.mapAttrs
+      mkDepOpt
+      (l.filterAttrs (_: enabled: enabled) config.depsRequired);


makeModule implementation concerns are leaking into the general interface. That's not good.

Shouldn't this be in a makeModule module?

Should deps be a freeform module?

Shouldn't this be in a makeModule module?

I thought explicitly specifying a list of required deps is something that we want everywhere. depsRequired seemed like the simplest way to do that. It's not supposed to be specific to makeModule.

Should deps be a freeform module?

Will this bring back the capability of specifying deps as a function?
I probably need to combine freeform and apply, I guess?

roberth · 2022-12-22T13:21:44Z

It was not possible to specify required dependencies explicitly, because it was not possible to create nested options likeoptions.deps.bash = mkOption ....

Did you try with mkOption { apply } instead of type = coercedTo?

roberth · 2022-12-22T13:24:34Z

should be renamed to inputs

Those could be understood to be flake inputs. Arguably those shouldn't be in scope though, so perhaps shadowing is a good thing. Still potentially confusing though. Especially inputs = { inputs, ... }: inputs.foo is a bit odd.

DavHau · 2022-12-22T13:59:11Z

I'm also surprised to see that pkgs is even in the package module arguments. Was that needed for something? (before this PR)

Which package module arguments do you mean?

Did you try with mkOption { apply } instead of type = coercedTo?

Not yet, but I will experiment with it now.

roberth · 2022-12-22T14:53:03Z

I'm also surprised to see that pkgs is even in the package module arguments. Was that needed for something? (before this PR)

Which package module arguments do you mean?

I think I inferred this wrong when reading the code. Maybe got confused by what you said is a bad example.

DavHau · 2022-12-22T14:53:46Z

It was not possible to specify required dependencies explicitly, because it was not possible to create nested options likeoptions.deps.bash = mkOption ....

Did you try with mkOption { apply } instead of type = coercedTo?

Which type would I pick that allows nested options? If I pick submodule, I cannot define a function for deps anymore, because it gets interpreted as a module, and errors out.

roberth · 2022-12-22T15:41:11Z

Which type would I pick that allows nested options? If I pick submodule, I cannot define a function for deps anymore, because it gets interpreted as a module, and errors out.

functionTo submodule? Or maybe the dependency sets could be specialArgs?

DavHau · 2022-12-22T17:04:49Z

To explain my though process a bit better:

One underlying question seems to be: Who gets to pick dependencies? The package maintainer, or the caller?
deps = { pkgs, ... }: puts the decision in the hand of the maintainer which makes sense in a mono repo like nixpkgs, where maintainers just want to add a file and have everything sorted.

Out in the decentralized wild, it might be more valuable to let the caller satisfy the dependencies, which requires that requirements are discoverable in some way.

Therefore I thought it's a good idea to enforce an option for each dependency.

Thinking about an end game where nix competes with language specific package managers, the decentralized fashion seems to be the more generic approach vs. nixpkgs is more specific. Maybe the logic of how dependencies are picked within nixpkgs, should be factored out into a separate module?

functionTo submodule? Or maybe the dependency sets could be specialArgs?

Genius! submoduleWith {modules = []; specialArgs = packageSets;} seems to work wonderfully. Thanks for that.

With that, I can get rid of depsDefault, as a mechanism to provide defaults.
depsRequired still seems necessary, if we want discoverable dependencies.

Maybe one can get rid of deps in most cases by providing a default implementation for deps that intersects depsRequired with a default dependencySet like pkgs.

A package module would then look like this:

{config, lib, ...}: {
  name = "test";
  # all required deps must always be declared
  depsRequired = {
    foo = true;
    bar = true;
    bash = true;
  };
  # `deps` is only required for picking non-default versions
  # This logic is specific to the repo the package resides in, but can easily
  #   be removed by a `mkForce`.
  deps = {pkgs, ...}: {
    # pick a special version for bar
    bar = pkgs.bar_2;
  };
}

roberth · 2022-12-22T21:40:25Z

Out in the decentralized wild, it might be more valuable to let the caller satisfy the dependencies, which requires that requirements are discoverable in some way.

I don't think the change will be huge. A lot of people already live in a decentralized wild (without Nix) and they rely on distributions to provide such things as docker base images. Even a gentoo user doesn't stitch together all dependencies by hand. (I think? It's been over a decade.)

depsRequired still seems necessary, if we want discoverable dependencies.

or mkOption, right?

depsRequired

types.enum [ true ];? types.submodule {}?
I never know what to make of sets of names.

  # `deps` is only required for picking non-default versions
  # This logic is specific to the repo the package resides in, but can easily
  #   be removed by a `mkForce`.
  deps = {pkgs, ...}: {
    # pick a special version for bar
    bar = pkgs.bar_2;
  };

Not sure if going in circles, but what about depsRequired = lib.attrNames (lib.lazyFunction config.deps (throw "The attribute set returned by depsmust have a static set of attribute names, so nooptionalAttrsor anything else that may depend on the arguments of thedeps function."))?

I should probably merge this, shouldn't I:

lib.lazyFunction: init NixOS/nixpkgs#194514

DavHau · 2022-12-24T10:17:12Z

I don't think the change will be huge. A lot of people already live in a decentralized wild (without Nix) and they rely on distributions to provide such things as docker base images. Even a gentoo user doesn't stitch together all dependencies by hand. (I think? It's been over a decade.)

Even if you have a community maintained package set, where the community picks one good version for everything, that approach only scales so far. There will always be the necessity to assemble environments with dependency versions different from the ones found in the community set. Language specific environments are the best example for that.

Customizing dependency trees can be done by hand or by a resolver, but in any case, to support this well, the requirements of a package should be discoverable.
Currently this is not the case in nixpkgs, which is one major reason for the big gap between the automated language2nix approaches and nixpkgs. Combining the two is hard. One is either in this world or in that world.

It would be nice if mixed approaches would be supported, so that users can satisfy their requirements, by taking some packages from nixpkgs and others from elsewhere (auto-generated, or different repos, etc...). Automatically.

Expanding a bit on the depsRequired idea, we could have, for example:

{
  depsRequired = {
    foo = "^2.0.0";
    bar = "*";
    baz = "~2.0";
  };
}

If packages had that, mixing dynamic language2nix approaches with nixpkgs would be a charm. Knowing more about a package is beneficial for automation. And automation is key to scale.

depsRequired still seems necessary, if we want discoverable dependencies.

or mkOption, right?

depsRequired is creating options for deps behind the scenes. It's equivalent of using mkOption. It just requires less typing.

Anyways, I might be biased by dealing with language2nix too much, and I do not want to pollute the core of drv-parts with anything that we don't agree is absolutely necessary.

DavHau · 2022-12-24T13:04:21Z

I now opened #11 which is a more minimal change, just allowing nested options for deps.
The new PR:

doesn't add any additional options like depsRequired etc.
keeps the deps = {pkgs, ...}: {inherit (deps) ...;} style intact

blaggacao · 2023-01-23T21:37:54Z

so this implementation / PR will be closed?

DavHau added 2 commits December 22, 2022 19:34

fix typos + reword

68782f6

roberth reviewed Dec 22, 2022

View reviewed changes

DavHau mentioned this pull request Jan 24, 2023

Can specialArgs be avoided? #14

Open

DavHau force-pushed the master branch from de34903 to 82ba415 Compare March 2, 2023 14:58

DavHau closed this Mar 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change mechanism for `deps`; add `depsRequired`; add `depsFrom`. #10

change mechanism for `deps`; add `depsRequired`; add `depsFrom`. #10

DavHau commented Dec 22, 2022 •

edited

Loading

roberth left a comment

roberth Dec 22, 2022

roberth Dec 22, 2022

roberth Dec 22, 2022

roberth Dec 22, 2022

DavHau Dec 22, 2022

roberth Dec 22, 2022

DavHau Dec 22, 2022

roberth commented Dec 22, 2022

roberth commented Dec 22, 2022

DavHau commented Dec 22, 2022

roberth commented Dec 22, 2022

DavHau commented Dec 22, 2022

roberth commented Dec 22, 2022

DavHau commented Dec 22, 2022

roberth commented Dec 22, 2022

DavHau commented Dec 24, 2022

DavHau commented Dec 24, 2022

blaggacao commented Jan 23, 2023

change mechanism for deps; add depsRequired; add depsFrom. #10

change mechanism for deps; add depsRequired; add depsFrom. #10

Conversation

DavHau commented Dec 22, 2022 • edited Loading

roberth left a comment

Choose a reason for hiding this comment

roberth Dec 22, 2022

Choose a reason for hiding this comment

roberth Dec 22, 2022

Choose a reason for hiding this comment

roberth Dec 22, 2022

Choose a reason for hiding this comment

roberth Dec 22, 2022

Choose a reason for hiding this comment

DavHau Dec 22, 2022

Choose a reason for hiding this comment

roberth Dec 22, 2022

Choose a reason for hiding this comment

DavHau Dec 22, 2022

Choose a reason for hiding this comment

roberth commented Dec 22, 2022

roberth commented Dec 22, 2022

DavHau commented Dec 22, 2022

roberth commented Dec 22, 2022

DavHau commented Dec 22, 2022

roberth commented Dec 22, 2022

DavHau commented Dec 22, 2022

To explain my though process a bit better:

roberth commented Dec 22, 2022

DavHau commented Dec 24, 2022

DavHau commented Dec 24, 2022

blaggacao commented Jan 23, 2023

change mechanism for `deps`; add `depsRequired`; add `depsFrom`. #10

change mechanism for `deps`; add `depsRequired`; add `depsFrom`. #10

DavHau commented Dec 22, 2022 •

edited

Loading