lib/types: dont warn loaOf for home-manager namespace #77575

worldofpeace · 2020-01-12T20:15:44Z

This option namespace is not a part of NixOS
so we shouldn't provide this warning for it.

Motivation for this change

Things done

This option namespace is not a part of NixOS so we shouldn't provide this warning for it.

rnhmjoj · 2020-01-12T20:49:44Z

I understand but I don't quite agree on this. Sure, home-manager is not a NixOS project, so it should be their responsibility to handle this. However it's a popular application among NixOS users and, I say, for the sake of UX we should provide clearer errors (especially if home-manager can't implement this, or can it?).

jtojnar · 2020-01-12T21:03:53Z

They can create their own loaOf fork something like this:

elemType: types.loaOf elemType // {
  merge = /* copy of our merge with attrNames replaced */;
}

However, I wonder if we should expose the merge function parametrizable by the attrNames, so that they could just do:

elemType: let
  origType = types.loaOf elemType;
in origType // {
  merge = origType._mergeWithAttrNames [
    /* home-manager attrNames */
  ];
}

cc @rycee

worldofpeace · 2020-01-12T23:19:04Z

I understand but I don't quite agree on this. Sure, home-manager is not a NixOS project, so it should be their responsibility to handle this. However it's a popular application among NixOS users and, I say, for the sake of UX we should provide clearer errors (especially if home-manager can't implement this, or can it?).

We should, as @jtojnar suggests, allow home-manager to utilize NixOS lib. it's reasonable to allow it to be extensible for other module systems that exist outside NixOS. It just shouldn't be specific to one single project.

jtojnar · 2020-01-13T00:06:11Z

Do not have time to test it right now but the following might be sufficient:

--- a/lib/types.nix
+++ b/lib/types.nix
@@ -329,57 +329,18 @@ rec {
     # List or attribute set of ...
     loaOf = elemType:
       let
-        convertAllLists = loc: defs:
+        convertAllLists = nameAttrs: loc: defs:
           let
             padWidth = stringLength (toString (length defs));
             unnamedPrefix = i: "unnamed-" + fixedWidthNumber padWidth i + ".";
           in
-            imap1 (i: convertIfList loc (unnamedPrefix i)) defs;
-        convertIfList = loc: unnamedPrefix: def:
+            imap1 (i: convertIfList nameAttrs loc (unnamedPrefix i)) defs;
+        anyString = placeholder "name";
+        convertIfList = nameAttrs: loc: unnamedPrefix: def:
           if isList def.value then
             let
               padWidth = stringLength (toString (length def.value));
               unnamed = i: unnamedPrefix + fixedWidthNumber padWidth i;
-              anyString = placeholder "name";
-              nameAttrs = [
-                { path = [ "environment" "etc" ];
-                  name = "target";
-                }
-                { path = [ "containers" anyString "bindMounts" ];
-                  name = "mountPoint";
-                }
-                { path = [ "programs" "ssh" "knownHosts" ];
-                  # hostNames is actually a list so we would need to handle it only when singleton
-                  name = "hostNames";
-                }
-                { path = [ "fileSystems" ];
-                  name = "mountPoint";
-                }
-                { path = [ "boot" "specialFileSystems" ];
-                  name = "mountPoint";
-                }
-                { path = [ "services" "znapzend" "zetup" ];
-                  name = "dataset";
-                }
-                { path = [ "services" "znapzend" "zetup" anyString "destinations" ];
-                  name = "label";
-                }
-                { path = [ "services" "geoclue2" "appConfig" ];
-                  name = "desktopID";
-                }
-                { path = [ "home-manager" "users" anyString "programs" "ssh" "matchBlocks" ];
-                  name = "host"; # https://github.com/rycee/home-manager/blob/e8dbc3561373b68d12decb3c0d7c1ba245f138f7/modules/programs/ssh.nix#L265
-                }
-                { path = [ "home-manager" "users" anyString "home" "file" ];
-                  name = "target"; # https://github.com/rycee/home-manager/blob/0e9b7aab3c6c27bf020402e0e2ef20b65c040552/modules/files.nix#L33
-                }
-                { path = [ "home-manager" "users" anyString "xdg" "configFile" ];
-                  name = "target"; # https://github.com/rycee/home-manager/blob/54de0e1d79a1370e57a8f23bef89f99f9b92ab67/modules/misc/xdg.nix#L41
-                }
-                { path = [ "home-manager" "users" anyString "xdg" "dataFile" ];
-                  name = "target"; # https://github.com/rycee/home-manager/blob/54de0e1d79a1370e57a8f23bef89f99f9b92ab67/modules/misc/xdg.nix#L58
-                }
-              ];
               matched = let
                 equals = a: b: b == anyString || a == b;
                 fallback = { name = "name"; };
@@ -430,12 +391,45 @@ rec {
               lib.warn msg res
           else
             def;
+
+        mergeLoaOf = rec {
+          _anyString = anyString;
+          __nameAttrs = [
+            { path = [ "environment" "etc" ];
+              name = "target";
+            }
+            { path = [ "containers" anyString "bindMounts" ];
+              name = "mountPoint";
+            }
+            { path = [ "programs" "ssh" "knownHosts" ];
+              # hostNames is actually a list so we would need to handle it only when singleton
+              name = "hostNames";
+            }
+            { path = [ "fileSystems" ];
+              name = "mountPoint";
+            }
+            { path = [ "boot" "specialFileSystems" ];
+              name = "mountPoint";
+            }
+            { path = [ "services" "znapzend" "zetup" ];
+              name = "dataset";
+            }
+            { path = [ "services" "znapzend" "zetup" anyString "destinations" ];
+              name = "label";
+            }
+            { path = [ "services" "geoclue2" "appConfig" ];
+              name = "desktopID";
+            }
+          ];
+          _mergeWithNameAttrs = nameAttrs: loc: defs: attrOnly.merge loc (convertAllLists nameAttrs loc defs);
+          __functor = self: _mergeWithNameAttrs __nameAttrs;
+        };
         attrOnly = attrsOf elemType;
       in mkOptionType rec {
         name = "loaOf";
         description = "list or attribute set of ${elemType.description}s";
         check = x: isList x || isAttrs x;
-        merge = loc: defs: attrOnly.merge loc (convertAllLists loc defs);
+        merge = mergeLoaOf;
         emptyValue = { value = {}; };
         getSubOptions = prefix: elemType.getSubOptions (prefix ++ ["<name?>"]);
         getSubModules = elemType.getSubModules;

misuzu · 2020-01-13T12:12:58Z

There is already a fix for this in home-manager: nix-community/home-manager#984

jtojnar · 2020-01-13T14:52:13Z

@misuzu that only fixes the warning producing definitions inside home-manager modules. If user sets any of those options in their home.nix to list, they will still see the warning.

@rycee will need to decide which of these variants they want:

improving the warning using _mergeWithAttrNames exposed from lib.types.loaOf as described above
improving the warning by overriding the whole merge method
ignoring the warning imprecision, letting users figure it out for themselves

rycee · 2020-01-13T21:32:13Z

All internal use of the list form of home.file should be removed from HM now. I'm not particularly concerned about the warning imprecision and instead rely on people carefully reading the release notes. I'll reconsider if I get many support requests 🙂

For the SSH hosts option I will have to ponder a bit. Most likely I'll try to figure out some nice way to introduce a dagOf option type since that is semantically the best for the option and it could possibly be made to be compatible with values of the attribute set type.

jtojnar · 2020-01-13T21:58:02Z

Well, dagOf could still be sub-type of attrsOf:

{
  a = { arrows = [ "b" "c" "d" "e" ]; };
  b = { arrows = [ "c" ]; };
  c = { arrows = [ "d" "e" ]; };
  d = { arrows = [ "e" ]; };
  e = { arrows = [ ]; };
}

lib/types: dont warn loaOf for home-manager namespace

265da70

This option namespace is not a part of NixOS so we shouldn't provide this warning for it.

worldofpeace requested a review from jtojnar January 12, 2020 20:15

worldofpeace requested review from edolstra, infinisil and nbp as code owners January 12, 2020 20:15

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jan 12, 2020

jtojnar approved these changes Jan 13, 2020

View reviewed changes

worldofpeace merged commit 441588c into NixOS:master Jan 13, 2020

worldofpeace deleted the home-manager-warnings-drop branch January 13, 2020 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib/types: dont warn loaOf for home-manager namespace #77575

lib/types: dont warn loaOf for home-manager namespace #77575

worldofpeace commented Jan 12, 2020

rnhmjoj commented Jan 12, 2020

jtojnar commented Jan 12, 2020 •

edited

worldofpeace commented Jan 12, 2020

jtojnar commented Jan 13, 2020 •

edited

misuzu commented Jan 13, 2020

jtojnar commented Jan 13, 2020

rycee commented Jan 13, 2020

jtojnar commented Jan 13, 2020

lib/types: dont warn loaOf for home-manager namespace #77575

lib/types: dont warn loaOf for home-manager namespace #77575

Conversation

worldofpeace commented Jan 12, 2020

Motivation for this change

Things done

rnhmjoj commented Jan 12, 2020

jtojnar commented Jan 12, 2020 • edited

worldofpeace commented Jan 12, 2020

jtojnar commented Jan 13, 2020 • edited

misuzu commented Jan 13, 2020

jtojnar commented Jan 13, 2020

rycee commented Jan 13, 2020

jtojnar commented Jan 13, 2020

jtojnar commented Jan 12, 2020 •

edited

jtojnar commented Jan 13, 2020 •

edited