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/nixos-option: fix evaluator to render a full submodule entry #75439

Merged
merged 4 commits into from Feb 1, 2020

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Dec 10, 2019

Motivation for this change

When running e.g. nixos-option users.users.ma27, the evaluation breaks
since ma27 is the attribute name in attrsOf (submodule {}), but not
a part of the option tree and therefore breaks with the following
errors:

error: At 'ma27' in path 'users.users.ma27': Attribute not found
An error occurred while looking for attribute names. Are you sure that 'users.users.ma27' exists?

This happens since the option evaluator expects that either the option
exists or the option is a submodule and the "next" token in the
attribute path points to an option (e.g. users.users.ma27.createHome).

This patch checks in the Attribute not found condition if the attribute-path
actually exists in the config tree. If that's true, a dummy-attrset is created
which contains {_type = "__nixos-option-submodule-attr";}, in that case, the
entire entry of the submodule will be displayed.

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 nix-review --run "nix-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.
Notify maintainers

cc @chkno

@chkno
Copy link
Member

chkno commented Dec 12, 2019

This patch doesn't fix this problem for me. I still get the same "Attribute not found" error message when I run nixos-option users.users.root with and without this change.

I agree that this should be fixed. I propose a slightly different way to fix it: chkno@35ac88d

(I'm not sure how the PR mechanics work from here. Do I start a separate pull request? Do you switch the source branch of this pull request from Ma27:submodule-fixes-for-nixos-option to chkno:submodule-fix-for-nixos-option? Do you copy that commit to this PR branch?)

When running e.g. `nixos-option users.users.ma27`, the evaluation breaks
since `ma27` is the attribute name in `attrsOf (submodule {})`, but not
a part of the option tree and therefore breaks with the following
errors:

```
error: At 'ma27' in path 'users.users.ma27': Attribute not found
An error occurred while looking for attribute names. Are you sure that 'users.users.ma27' exists?
```

This happens since the option evaluator expects that either the option
exists or the option is a submodule and the "next" token in the
attribute path points to an option (e.g. `users.users.ma27.createHome`).

This patch checks in the `Attribute not found` condition if the attribute-path
actually exists in the config tree. If that's true, a dummy-attrset is created
which contains `{_type = "__nixos-option-submodule-attr";}`, in that case, the
entire entry of the submodule will be displayed.
@Ma27 Ma27 force-pushed the submodule-fixes-for-nixos-option branch from fddf656 to e0780c5 Compare December 12, 2019 13:27
@Ma27
Copy link
Member Author

Ma27 commented Dec 12, 2019

Oof.... you're right, that was an avoidable mistake (it worked in my case due to a coincidence :/). Updated the code now. (I'd prefer to have the attribute printed when running nixos-option users.users.ma27 tbh).

Regarding contributions from your side: feel free to open a PR against this branch on my fork :)

@chkno
Copy link
Member

chkno commented Dec 12, 2019

I created Ma27#2 to pull that change into this PR.

@Ma27 Ma27 requested a review from lheckemann December 14, 2019 02:22
1. This makes aggregates of submodules (including the very important
"nixos-option users.users.<username>" case) behave the same way as any
other you-need-to-keep-typing-to-get-to-an-option-leaf (eg:
"nixos-option environment").

Before e0780c5:

  $ nixos-option users.users.root
  error: At 'root' in path 'users.users.root': Attribute not found
  An error occurred while looking for attribute names. Are you sure that 'users.users.root' exists?

After e0780c5 but before this change, this query just printed out a raw
thing, which is behavior that belongs in "nix eval", "nix-instantiate
--eval", or "nix repl <<<":

  $ nixos-option users.users.root
  {
    _module = {
      args = { name = "root"; };
      check = true;
    };
    createHome = false;
    cryptHomeLuks = null;
    description = "System administrator";
    ...

After this change:

  $ nixos-option users.users.root
  This attribute set contains:
  createHome
  cryptHomeLuks
  description
  extraGroups
  group
  hashedPassword
  ...

2. For aggregates of other types (not submodules), print out the option
that contains them rather than printing an error message.

Before:

  $ nixos-option environment.shellAliases.l
  error: At 'l' in path 'environment.shellAliases.l': Attribute not found
  An error occurred while looking for attribute names. Are you sure that 'environment.shellAliases.l' exists?

After:

  $ nixos-option environment.shellAliases.l
  Note: showing environment.shellAliases instead of environment.shellAliases.l
  Value:
  {
    l = "ls -alh";
    ll = "ls -l";
    ls = "ls --color=tty";
  }
  ...
Copy link
Member

@lheckemann lheckemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this and it works nicely, but I haven't reviewed the code changes.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/99

@Ma27 Ma27 merged commit 7f49fa6 into NixOS:master Feb 1, 2020
@Ma27 Ma27 deleted the submodule-fixes-for-nixos-option branch February 1, 2020 09:01
anna328p pushed a commit to anna328p/nixpkgs that referenced this pull request Feb 2, 2020
…tion

nixos/nixos-option: fix evaluator to render a full submodule entry
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants