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 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 Ma27:submodule-fixes-for-nixos-option branch from fddf656 to e0780c5 Dec 12, 2019
@Ma27
Copy link
Member Author

@Ma27 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 chkno commented Dec 12, 2019

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

@Ma27 Ma27 requested a review from lheckemann Dec 14, 2019
chkno added 3 commits Dec 12, 2019
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";
  }
  ...
@chkno
chkno approved these changes Dec 19, 2019
Copy link
Member

@lheckemann lheckemann left a comment

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

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Dec 26, 2019

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
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
@Ma27 Ma27 deleted the Ma27:submodule-fixes-for-nixos-option branch Feb 1, 2020
anna328p added 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

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