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

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

Merged
merged 1 commit into from Jan 13, 2020

Conversation

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Jan 12, 2020

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

Motivation for this change

#77501 #77501 (comment).

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.
This option namespace is not a part of NixOS
so we shouldn't provide this warning for it.
@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Jan 12, 2020

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
Copy link
Contributor

@jtojnar jtojnar commented Jan 12, 2020

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
Copy link
Member Author

@worldofpeace worldofpeace commented Jan 12, 2020

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
Copy link
Contributor

@jtojnar jtojnar commented Jan 13, 2020

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
Copy link
Contributor

@misuzu misuzu commented Jan 13, 2020

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

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Jan 13, 2020

@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
Copy link
Member

@rycee rycee commented Jan 13, 2020

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
Copy link
Contributor

@jtojnar jtojnar commented Jan 13, 2020

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 = [ ]; };
}

DAG

@worldofpeace worldofpeace merged commit 441588c into NixOS:master Jan 13, 2020
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@worldofpeace worldofpeace deleted the worldofpeace:home-manager-warnings-drop branch Jan 13, 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

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