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

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

Merged
merged 4 commits into from Jan 12, 2020

Conversation

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Jan 11, 2020

cc #77189

@jtojnar jtojnar requested a review from peti as a code owner Jan 11, 2020
@jtojnar
Copy link
Contributor Author

@jtojnar jtojnar commented Jan 11, 2020

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 jtojnar requested review from edolstra, Infinisil and nbp as code owners Jan 11, 2020
@jtojnar
Copy link
Contributor Author

@jtojnar jtojnar commented Jan 11, 2020

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

@jtojnar jtojnar commented Jan 11, 2020

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

@jtojnar jtojnar commented Jan 11, 2020

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

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

This comment has been minimized.

@rnhmjoj

rnhmjoj Jan 11, 2020
Contributor

Neat! I didn't think about this.

@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Jan 11, 2020

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

@jtojnar jtojnar commented Jan 11, 2020

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

@rycee rycee commented Jan 11, 2020

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.

jtojnar added 2 commits Jan 11, 2020
Now we suggest correct names for all options in Nixpkgs and also home-manager at the time of writing.
@jtojnar
Copy link
Contributor Author

@jtojnar jtojnar commented Jan 11, 2020

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

@rnhmjoj rnhmjoj commented Jan 11, 2020

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).

{ 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
}
Comment on lines +370 to +381

This comment has been minimized.

@worldofpeace

worldofpeace Jan 11, 2020
Member

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

@worldofpeace worldofpeace commented Jan 11, 2020

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

@jtojnar jtojnar commented Jan 11, 2020

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 jtojnar merged commit 61cf52b into NixOS:master Jan 12, 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
@jtojnar jtojnar deleted the jtojnar:more-loaof-fxes branch Jan 12, 2020
@jtojnar
Copy link
Contributor Author

@jtojnar jtojnar commented Jan 12, 2020

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

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Jan 12, 2020

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 👍

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

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