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

nixos/nixpkgs: add override/merging capabilities from the module system to the `nixpkgs.config` option #80582

Closed
wants to merge 1 commit into from

Conversation

@Ma27
Copy link
Member

@Ma27 Ma27 commented Feb 20, 2020

Motivation for this change

The nixpkgs.config[1] option is just a plain attribute-set that is directly
passed to <nixpkgs/pkgs/top-level/default.nix>. This has the following
drawbacks:

  • Missing override/merging capabilities: you can't use module-system features
    like mkOverride/mkOrder/mkIf/etc.

  • Conflicting definitions don't cause an evaluation error. The merging
    is basically a fold recursiveUpdate {} defs which means that the
    last declaration of a value inside nixpkgs.config "wins".

    For example,

    { pkgs, lib, ... }: {
      nixpkgs = lib.mkMerge [
        { config.allowUnfree = false; }
        { config.allowUnfree = true; }
      ];
      environment.systemPackages = [
        pkgs.spotify /* an unfree package */
      ];
    }

    would evaluate.

Another attempt to use the module-system for nixpkgs-config stalled
after a long discussion[2].

This change aims to be minimally-invasive by applying the filters from
<nixpkgs/lib/modules.nix> onto the attribute-set without constraining
the attributes with a predefined option structure (or in other words:
you don't need to change anything in the module-system to add a new
nixpkgs-config value).

This is done by replacing the old configType with a new option-type
that emulates the merging behavior of the module-system:

  • All values are merged together into a single attr-set to ensure that
    all values that are defined in any included module are taken into
    account.

  • Each sub-attribute that is an attr-set and has a _type-field (which
    defines which mk*-filter was used for the declaration) will be
    passed through priority/discharge/ordering filters to resolve all
    mk* statements in nixpkgs.config.

  • The result will be merged together to ensure that no conflicting
    definitions exist while sequential values will be merged together.

Due to the validation mechanisms from the module-system, the following
case will break with an evaluation error now:

  • When declaring values of different types to a single nixpkgs-config
    option, the eval will fail now (rather than silently replacing the
    values in lib.recursiveUpdate):

    For example,

    { lib, ... }: {
      nixpkgs = lib.mkMerge [
        { config.chromium.enableWidevine = true; }
        { config.chromium = 23; }
      ];
    }

    will break with the following error:

    Cannot merge definitions of `nixpkgs.config.chromium' given in `<unknown-file>' and `<unknown-file>'.
    

    (Please note that there might be some more edge-cases I'm just missing
    while writing the commit-message).

Additionally the following issues aren't fixed yet:

  • There's no support for anything like types.separatedString. As
    string-concatenation during merge-phase may be harmful[3] (and there's no
    way to differentiate a separatedString from a "normal" str without
    using <nixpkgs/lib/types.nix>), evaluations like this will always
    fail for now:

    { lib, ... }: {
      nixpkgs = lib.mkMerge [
        { config.random = ''
            foo
          '';
        }
        { config.random = ''
            bar
          '';
        }
      ];
    }
    The unique option `nixpkgs.config.random' is defined multiple times, in:
     - <unknown-file>
     - <unknown-file>.
    

    Please note that the only reason why this yields <unknown-file> is
    because the values were defined in a mkMerge. Since loc is passed
    properly to the error-handler, the actual file where a value is
    declared would be shown if the options were defined in different
    files.

  • Expressions inside a mkMerge aren't evaluated yet. For instance, in
    the following expression not all mk* would be resolved properly:

    { lib, ... }: {
      nixpkgs.config = {
        chromium = lib.mkMerge [
          { randomValue = mkIf true "something"; }
        ];
      };
    }
    { chromium = { randomValue = { _type = "if"; condition = true; content = "something"; }; }; }
    

    Please note that this only applies to ordering & conditionals, overrides
    inside a mkMerge are resolved without any problems.

[1] https://nixos.org/nixpkgs/manual/#idm140737322665904
[2] #56227
[3] 03391cd

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
…em to the `nixpkgs.config` option

The `nixpkgs.config`[1] option is just a plain attribute-set that is directly
passed to `<nixpkgs/pkgs/top-level/default.nix>`. This has the following
drawbacks:

* Missing override/merging capabilities: you can't use module-system features
  like `mkOverride`/`mkOrder`/`mkIf`/etc.

* Conflicting definitions don't cause an evaluation error. The merging
  is basically a `fold recursiveUpdate {} defs` which means that the
  last declaration of a value inside `nixpkgs.config` "wins".

  For example,

  ``` nix
  { pkgs, lib, ... }: {
    nixpkgs = lib.mkMerge [
      { config.allowUnfree = false; }
      { config.allowUnfree = true; }
    ];
    environment.systemPackages = [
      pkgs.spotify /* an unfree package */
    ];
  }
  ```

  would evaluate.

Another attempt to use the module-system for nixpkgs-config stalled
after a long discussion[2].

This change aims to be minimally-invasive by applying the filters from
`<nixpkgs/lib/modules.nix>` onto the attribute-set without constraining
the attributes with a predefined option structure (or in other words:
you don't need to change anything in the module-system to add a new
nixpkgs-config value).

This is done by replacing the old `configType` with a new option-type
that emulates the merging behavior of the module-system:

* All values are merged together into a single attr-set to ensure that
  all values that are defined in any included module are taken into
  account.

* Each sub-attribute that is an attr-set and has a `_type`-field (which
  defines which `mk*`-filter was used for the declaration) will be
  passed through priority/discharge/ordering filters to resolve all
  `mk*` statements in `nixpkgs.config`.

* The result will be merged together to ensure that no conflicting
  definitions exist while sequential values will be merged together.

Due to the validation mechanisms from the module-system, the following
case will break with an evaluation error now:

* When declaring values of different types to a single nixpkgs-config
  option, the eval will fail now (rather than silently replacing the
  values in `lib.recursiveUpdate`):

  For example,

  ``` nix
  { lib, ... }: {
    nixpkgs = lib.mkMerge [
      { config.chromium.enableWidevine = true; }
      { config.chromium = 23; }
    ];
  }
  ```

  will break with the following error:

  ```
  Cannot merge definitions of `nixpkgs.config.chromium' given in `<unknown-file>' and `<unknown-file>'.
  ```

  (Please note that there might be some more edge-cases I'm just missing
  while writing the commit-message).

Additionally the following issues aren't fixed yet:

* There's no support for anything like `types.separatedString`. As
  string-concatenation during merge-phase may be harmful[3] (and there's no
  way to differentiate a separatedString from a "normal" str without
  using `<nixpkgs/lib/types.nix>`), evaluations like this will always
  fail for now:

  ``` nix
  { lib, ... }: {
    nixpkgs = lib.mkMerge [
      { config.random = ''
          foo
        '';
      }
      { config.random = ''
          bar
        '';
      }
    ];
  }
  ```

  ```
  The unique option `nixpkgs.config.random' is defined multiple times, in:
   - <unknown-file>
   - <unknown-file>.
  ```

  Please note that the only reason why this yields `<unknown-file>` is
  because the values were defined in a `mkMerge`. Since `loc` is passed
  properly to the error-handler, the actual file where a value is
  declared would be shown if the options were defined in different
  files.

* Expressions inside a `mkMerge` aren't evaluated yet. For instance, in
  the following expression not all `mk*` would be resolved properly:

  ``` nix
  { lib, ... }: {
    nixpkgs.config = {
      chromium = lib.mkMerge [
        { randomValue = mkIf true "something"; }
      ];
    };
  }
  ```

  ```
  { chromium = { randomValue = { _type = "if"; condition = true; content = "something"; }; }; }
  ```

  Please note that this only applies to ordering & conditionals, overrides
  inside a `mkMerge` are resolved without any problems.

[1] https://nixos.org/nixpkgs/manual/#idm140737322665904
[2] #56227
[3] 03391cd
Copy link
Member Author

@Ma27 Ma27 left a comment

I know that this change is rather hacky, but IMHO it's the simplest solution to enable module-system features in nixpkgs.config without breaking too much existing code. If you know better approaches or found some edge-cases that break with this patch, please let me know, I'm happy to take a look at this (and probably fix those), but please take a look at the commit-message first, I may have thought about your issue already.

@Infinisil you're one of the folks where I know for sure that they know fairly much about the module-system, so I hope it's okay that I requested a review from you here as well :)

#
# FIXME This is missing support for separatedString!
merge = let h = (head result).value or null; in
if isList h || isAttrs h then mergeDefaultOption else mergeOneOption;

This comment has been minimized.

@Ma27

Ma27 Feb 20, 2020
Author Member

As mentioned in the comment, separatedString doesn't work here. As recursiveUpdate did silent overrides, this never worked before, but are there any suggestions how to fix this as well? :)

appendVal = all: value_: next:
let
# FIXME support `mkMerge [{ snens = mkIf cond val; }]`
discharged = dischargeProperties value_;

This comment has been minimized.

@Ma27

Ma27 Feb 20, 2020
Author Member

As mentioned in the comment, mk{If,Merge,Before,...} aren't evaluated inside a mkMerge (mkMerge [ (mkIf cond { /* ... */ } ]) works fine though). IMHO this would make the code even more complex, but are there any suggestions how fix this? :)

let
# Ensure that each attribute from nixpkgs.config appears at the
# first iteration to make sure that nothing is lost.
allValues = fold recursiveUpdate {} defs;

This comment has been minimized.

@Ma27

Ma27 Feb 20, 2020
Author Member

I'm aware that this is a gross hack to make sure that any attr-names that are defined in any moduel taken into account is known to the merger. Are there any suggestions for a better workaround? :)


# Gather all declarations of a value in the `nixpkgs.config` attr-set.
allDeclarations = foldl
(all: next: appendVal all (optCall (attrByPath path null next) finalPkgs) next)

This comment has been minimized.

@Ma27

Ma27 Feb 20, 2020
Author Member

The optCall in the recursive mapAttrs replaces the merging behavior for {perlP,p}ackageOverrides. Please note that I'm now using finalPkgs rather than pkgs as this seems more appropriate to me (since finalPkgs also takes overlays into account). If this is a wrong decision, please let me know, we probably might want to use pkgs rather than finalPkgs here.

@roberth
Copy link
Contributor

@roberth roberth commented Mar 8, 2020

What we need here is a new module system feature. Being able to define unspecified options is more generally useful. For example

  • as an upgrade path from types.attrsOf types.unspecified to types.submoduleWith
  • as a better extraConfig NixOS/rfcs#42. @Infinisil

By supporting it in the module system, we don't have to maintain one or more hacky reimplementations of this feature.

@Ma27
Copy link
Member Author

@Ma27 Ma27 commented Mar 8, 2020

By supporting it in the module system, we don't have to maintain one or more hacky reimplementations of this feature.

So you're basically suggesting that this should be made generally useful, right? I don't see a problem with it, I kept it in the nixpkgs-module for now as a POC :)

@Ma27 Ma27 requested a review from lheckemann Mar 12, 2020
@Infinisil
Copy link
Member

@Infinisil Infinisil commented Mar 12, 2020

I just gave implementing partially typed options a go. The basic use-case works, but the semantics of this implementation are inconsistent. The work is in this branch: https://github.com/Infinisil/nixpkgs/tree/partially-typed, and an example of using it:

{ pkgs, config, lib, ... }: with lib;
{
  options.foo = lib.mkOption {
    type = with types; types.partialSubmodule [ bool int ] ({ config, ... }: {
      options.bar = lib.mkOption {
        type = bool;
        default = false;
      };

      options.b = lib.mkOption {
        type = types.int;
        default = 0;
      };

      # Doesn't work
      config.test = mkIf config.bar "test";

      # Doesn't work
      config.a = config.baz;
    });
  };

  config.foo = {
    bar = true;
    baz = 10;
  };
}

I implemented it by writing a type that combines two other types (one of which is an attrsOf and the other a submodule), requiring that both type check, merging them both separately, then merging the results of both. This has the disadvantage that typed and untyped values can't interact with each other (as shown in the example above).

So I think a better approach to implement this would be to dynamically generate internal options for all untyped values that are specified, such that doing baz = 10; generates options.baz = mkOption { type = types.int; internal = true; }; config.baz = 10; and then merging that with the typed submodule in a single evalModules invocation.

I might give this a go too soon. If this would work well, it could simplify NixOS/rfcs#42 considerably.

@roberth
Copy link
Contributor

@roberth roberth commented Mar 12, 2020

You could rename combination to intersection.
However, I think it's more ergonomic to keep the option values outside of the generic attributes, which combination/intersection doesn't allow by itself, because it can't know which is which. So perhaps it's better to make the module system return the unused definitions and allow those to be typed and merged?

submoduleWith {
  allowUndeclaredAs = attrsOf string; # or something like `atoms` from the branch
  mergeUndeclared = recursiveUpdate; # or opts: attrs: opts // { _undeclared = attrs; }
  ...
}

The module system can then gather up the undeclared definitions, check and merge them. The default value for allowUndeclaredAs is the current check: no definitions allowed.
mergeUndeclared is like resultMerge.
So the difference this achieves is that attrsOf string is applied only to the generic attributes, so that mergeUndeclared doesn't need to duplicate logic to tolerate option values for things like packageOverrides.

I liked the elegance of something like combination, but it seems like this and addCheck already cover its use cases.

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Mar 15, 2020

I did another run for this, and I seem to have a nicely working prototype now! The work is in https://github.com/Infinisil/nixpkgs/tree/partially-typed-v2, and here's an example of how it looks:

let
  lib = import ./lib;
  inherit (lib) types;
in

lib.evalModules {
  modules = [
    ({ lib, config, ... }: {
      options.services.foo = {
        enable = lib.mkEnableOption "service";

        settings = lib.mkOption {
          type = with types; submoduleWith {
            unstructuredOption = lib.mkOption {
              type = oneOf [ str int ];
            };
            modules = [{
              options.port = lib.mkOption {
                type = types.int;
              };
            }];
          };
          default = {};
        };
      };

      options.result = lib.mkOption {
        type = types.str;
        default = "";
      };

      config = lib.mkIf config.services.foo.enable {

        services.foo.settings.bar = lib.mkDefault "bar";

        result = lib.concatStringsSep "\n" (lib.mapAttrsToList (name: value: "${name} = ${toString value}") config.services.foo.settings);

      };
    })
    {
      services.foo = {
        enable = true;
        settings.port = 80;
        # This throws an error that port isn't of type int
        # settings.port = "80";
        settings.some-value = "value";
        settings.bar = lib.mkForce 10;
      };
    }
  ];
}

This now works directly through the module system: Instead of it only trying to evaluate declared options, it now also looks at all definitions without a declaration (if unstructuredOption != null) and pretends they have a single declaration of the option unstructuredOption. The semantics for recursive types I haven't figured out properly yet, but overall it seems to work really well, even with more complicated scenarios of options depending on others.

@roberth
Copy link
Contributor

@roberth roberth commented Mar 16, 2020

unstructuredOption

If you bundle them up and treat them as a single, potentially nested attribute set, you can leave the choice of nesting semantics to the caller by means of their choice of type for the whole attrset. This also gives the caller more power over merging.
For the implementation, this probably means you have to gather those options separately and you have to add them back in at the last minute, in a special attribute like config._unstructured or by means of a caller-supplied function unstructured -> config -> config

@Infinisil Infinisil mentioned this pull request Mar 16, 2020
4 of 4 tasks complete
@Ma27
Copy link
Member Author

@Ma27 Ma27 commented May 22, 2020

Since the fallback-type patches seem like a better and more mature idea to me, I'm going to close this for now.

@Ma27 Ma27 closed this May 22, 2020
@Ma27 Ma27 deleted the Ma27:nixpkgs-config-mergable branch May 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.