tree-wide: fix more warning related to loaOf deprecation #77501

jtojnar · 2020-01-11T07:11:44Z

jtojnar · 2020-01-11T07:13:59Z

Some options do not use name property, leading to a wrong warning message:

trace: warning: In file /home/jtojnar/Projects/nixpkgs/nixos/modules/services/mail/postfix.nix
a list is being assigned to the option config.environment.etc.
This will soon be an error as type loaOf is deprecated.
See https://git.io/fj2zm for more information.
Do
  environment.etc =
    { unnamed = {...}; ...}
instead of
  environment.etc =
    [ { name = "unnamed"; ...} ...]

error:

Not all modules use name attribute as the name of the submodule, for example, environment.etc uses target. We will need to maintain a list of exceptions.

jtojnar · 2020-01-11T08:05:28Z

Improved the warning message. With the last commit cherry-picked my config warns:

trace: warning: In file /etc/nixos/configuration.nix
a list is being assigned to the option config.boot.initrd.luks.devices.
This will soon be an error as type loaOf is deprecated.
See https://git.io/fj2zm for more information.
Do
  boot.initrd.luks.devices =
    { root = {...}; ...}
instead of
  boot.initrd.luks.devices =
    [ { name = "root"; ...} ...]

trace: warning: In file /home/jtojnar/Projects/nixpkgs/nixos/modules/services/mail/postfix.nix
a list is being assigned to the option config.environment.etc.
This will soon be an error as type loaOf is deprecated.
See https://git.io/fj2zm for more information.
Do
  environment.etc =
    { postfix = {...}; ...}
instead of
  environment.etc =
    [ { target = "postfix"; ...} ...]

cc @worldofpeace @rnhmjoj @petabyteboy

jtojnar · 2020-01-11T08:39:34Z

I inspected the nixos/ directory and saw the following attrNames:

"containers.<name>.bindMounts" = "mountPoint"; # nested inside containers attrsOf so simple attrNames.${option} will not work
"programs.ssh.knownHosts" = "hostNames"; # hostNames is actually a list so we would need to handle it only when singleton
"fileSystems" = "mountPoint";
"boot.specialFileSystems" = "mountPoint";
"services.znapzend.zetup" = "dataset";
"services.znapzend.zetup.<dataset>.destinations" = "label"; # nested inside services.znapzend.zetup loaOf so simple attrNames.${option} will not work
"services.geoclue2.appConfig" = "desktopID";

Not sure if it is worth adding them to the list.

jtojnar · 2020-01-11T09:11:04Z

home-manager also suffers from this but the options are apparently prefixed by something like home-manager.users.foo so we would need to handle

"<home-manager-prefix>.programs.ssh.matchBlocks" = "host"; # https://github.com/rycee/home-manager/blob/e8dbc3561373b68d12decb3c0d7c1ba245f138f7/modules/programs/ssh.nix#L265
"<home-manager-prefix>.home.file" = "target"; # https://github.com/rycee/home-manager/blob/0e9b7aab3c6c27bf020402e0e2ef20b65c040552/modules/files.nix#L33
"<home-manager-prefix>.xdg.configFile" = "target"; # https://github.com/rycee/home-manager/blob/54de0e1d79a1370e57a8f23bef89f99f9b92ab67/modules/misc/xdg.nix#L41
"<home-manager-prefix>.xdg.dataFile" = "target"; # https://github.com/rycee/home-manager/blob/54de0e1d79a1370e57a8f23bef89f99f9b92ab67/modules/misc/xdg.nix#L58

cc @rycee

rnhmjoj · 2020-01-11T10:16:26Z

lib/types.nix

@@ -340,18 +340,22 @@ rec {
            let
              padWidth = stringLength (toString (length def.value));
              unnamed = i: unnamedPrefix + fixedWidthNumber padWidth i;
+              nameAttrs = {


Neat! I didn't think about this.

rnhmjoj · 2020-01-11T10:24:35Z

Not sure if it is worth adding them to the list.

Why not? It won't hurt.

home-manager also suffers from this

rycee mentioned they were using loaOf but never reply when I asked if home-manager could define and use its own loaOf type. If it's not possible (I don't know much about home-manager) we could still make a exception for home-manager.

jtojnar · 2020-01-11T10:34:00Z

Why not? It won't hurt.

The matching logic will become more complicated than just attrNames.${option}.

rycee mentioned they were using loaOf but never reply when I asked if home-manager could define and use its own loaOf type.

IMHO it would make sense to have home-manager follow the same idioms as NixOS module tree – their options are equivalent to environment.etc. So deprecate the list style definitions for now, and switch to types.attrsOf after 19.03 branch-off.

rycee · 2020-01-11T12:02:57Z

The home.file, xdg.configFile, xdg.dataFile options correspond to environment.etc and can be handled in the same way. The programs.ssh.matchBlocks option is trickier and I'm not certain what the best solution would be. It could be turned back into a listOf, the type it had originally, but I suspect the proper type should be something along the lines of dagOf.

Now we suggest correct names for all options in Nixpkgs and also home-manager at the time of writing.

jtojnar · 2020-01-11T14:11:53Z

Now we suggest correct names for all options in Nixpkgs and also home-manager at the time of writing.

But damn it is ugly.

To test, you can use the following test.nix

{...}: {
  containers."foo.boar".bindMounts = [{ mountPoint = "foo"; }];
  programs.ssh.knownHosts = [
    { hostNames = ["foo"]; }
    { hostNames = [ "bar" "baz"]; }
  ];
  fileSystems = [
    { mountPoint = "/"; }
    { mountPoint = "/boot"; }
  ];
  boot.initrd.luks.devices = [ { name = "root"; } ];
}

and run nixos-option -I nixos-config=$PWD/test.nix containers programs.ssh.knownHosts fileSystems boot.initrd.luks.devices

rnhmjoj · 2020-01-11T14:16:22Z

But damn it is ugly.

It's complicated but it's not that ugly. I think it's worth it considering it will make it easy for the users to figure out what's going on. Also this code will be removed soon enough (hopefully in 20.09).

worldofpeace · 2020-01-11T16:57:17Z

lib/types.nix

+                { 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
+                }


Home-manager isn't a nixos project, and while it would be very nice of us to try to not cause confusion over there it isn't our responsibility to have this in lib/types.nix. It just seems problematic, even if it isn't a difficult thing to do.

worldofpeace · 2020-01-11T16:59:18Z

Also this code will be removed soon enough (hopefully in 20.09).

That would be a breaking change then, right?
I think those are more difficult to span out for one release.

jtojnar · 2020-01-11T18:04:56Z

That would be a breaking change then, right?

Yes. The options will be changed to attrsOf some time after 20.03 branch-off and loaOf will be deleted.

I think those are more difficult to span out for one release.

Given that our maintenance periods overlap only in immediately successive releases, there will be 6 months to fix this warning on stable and 2 months on unstable. I would call that sufficient.

jtojnar · 2020-01-12T17:48:08Z

Let's just merge this as a stop gap, we can improve it later.

worldofpeace · 2020-01-12T20:19:14Z

That would be a breaking change then, right?

Yes. The options will be changed to attrsOf some time after 20.03 branch-off and loaOf will be deleted.

I think those are more difficult to span out for one release.

Given that our maintenance periods overlap only in immediately successive releases, there will be 6 months to fix this warning on stable and 2 months on unstable. I would call that sufficient.

No problem with that 👍

tree-wide: fix more warning related to loaOf deprecation

e85b34c

jtojnar requested a review from peti as a code owner January 11, 2020 07:11

ofborg bot added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jan 11, 2020

lib/types: improve loaOf warning

b0c2c96

Not all modules use name attribute as the name of the submodule, for example, environment.etc uses target. We will need to maintain a list of exceptions.

jtojnar requested review from edolstra, infinisil and nbp as code owners January 11, 2020 07:57

rnhmjoj reviewed Jan 11, 2020

View reviewed changes

jtojnar added 2 commits January 11, 2020 13:32

lib/types: only show ... in loaOf warning when necessary

6fc46fb

lib/types: improve loaOf message even more

13633bd

Now we suggest correct names for all options in Nixpkgs and also home-manager at the time of writing.

worldofpeace reviewed Jan 11, 2020

View reviewed changes

jtojnar merged commit 61cf52b into NixOS:master Jan 12, 2020

jtojnar deleted the more-loaof-fxes branch January 12, 2020 17:47

worldofpeace mentioned this pull request Jan 12, 2020

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

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tree-wide: fix more warning related to loaOf deprecation #77501

tree-wide: fix more warning related to loaOf deprecation #77501

jtojnar commented Jan 11, 2020

jtojnar commented Jan 11, 2020

jtojnar commented Jan 11, 2020

jtojnar commented Jan 11, 2020 •

edited

jtojnar commented Jan 11, 2020

rnhmjoj Jan 11, 2020

rnhmjoj commented Jan 11, 2020

jtojnar commented Jan 11, 2020

rycee commented Jan 11, 2020

jtojnar commented Jan 11, 2020

rnhmjoj commented Jan 11, 2020

worldofpeace Jan 11, 2020

worldofpeace commented Jan 11, 2020

jtojnar commented Jan 11, 2020

jtojnar commented Jan 12, 2020

worldofpeace commented Jan 12, 2020

Navigation Menu

tree-wide: fix more warning related to loaOf deprecation #77501

tree-wide: fix more warning related to loaOf deprecation #77501

Conversation

jtojnar commented Jan 11, 2020

jtojnar commented Jan 11, 2020

jtojnar commented Jan 11, 2020

jtojnar commented Jan 11, 2020 • edited

jtojnar commented Jan 11, 2020

rnhmjoj Jan 11, 2020

Choose a reason for hiding this comment

rnhmjoj commented Jan 11, 2020

jtojnar commented Jan 11, 2020

rycee commented Jan 11, 2020

jtojnar commented Jan 11, 2020

rnhmjoj commented Jan 11, 2020

worldofpeace Jan 11, 2020

Choose a reason for hiding this comment

worldofpeace commented Jan 11, 2020

jtojnar commented Jan 11, 2020

jtojnar commented Jan 12, 2020

worldofpeace commented Jan 12, 2020

jtojnar commented Jan 11, 2020 •

edited