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

Clarify error message of 'assigning to top-level attribute' #76702

Merged
merged 1 commit into from Jan 8, 2020

Conversation

@raboof
Copy link
Contributor

@raboof raboof commented Dec 30, 2019

If I understand correctly, the problem isn't so much that you're assigning to
that top-level attribute, but that the assignment to the attribute (or any
child of the attribute) introduces the 'config' object and prevents 'lifting'
all settings to a generated 'config' object.

Motivation for this change

I think this would improve the new-user experience, where you're not yet so
sure where configuration options should go.

@danbst
Copy link
Contributor

@danbst danbst commented Dec 30, 2019

@raboof did you get this error when you tried to add some options. to your module?

I think this is most common case. And perhaps we can solve it the other way round - support options. inside config. Something like

diff --git a/lib/modules.nix b/lib/modules.nix
index 44db77b5d1c..482cba3b4fb 100644
--- a/lib/modules.nix
+++ b/lib/modules.nix
@@ -120,7 +120,7 @@ rec {
       then { meta = m.meta; }
       else {};
     in
-    if m ? config || m ? options then
+    if m ? config then
       let badAttrs = removeAttrs m ["_file" "key" "disabledModules" "imports" "options" "config" "meta"]; in
       if badAttrs != {} then
         throw "Module `${key}' has an unsupported attribute `${head (attrNames badAttrs)}'. This is caused by assignments to the top-level attr
ibutes `config' or `options'."
@@ -137,8 +137,8 @@ rec {
         key = toString m.key or key;
         disabledModules = m.disabledModules or [];
         imports = m.require or [] ++ m.imports or [];
-        options = {};
-        config = mkMerge [ (removeAttrs m ["_file" "key" "disabledModules" "require" "imports"]) metaSet ];
+        options = m.options or {};
+        config = mkMerge [ (removeAttrs m ["_file" "key" "disabledModules" "require" "imports" "options" ]) metaSet ];
       };
 
   applyIfFunction = key: f: args@{ config, options, lib, ... }: if isFunction f then

@nbp @Infinisil what do you think?

@raboof
Copy link
Contributor Author

@raboof raboof commented Dec 31, 2019

@danbst I ran into this when I added config.allowUnfreePredicate to my /etc/nixos/configuration.nix - in that case it should have been nixpkgs.config.allowUnfreePrecidate. This adapted message is still a bit confusing, but seemed like an improvement. I'm still a bit of a nixos newbie, so perhaps there is an even better way to express this, but at least I wanted to propose an improvement before I lose my "beginners' mind" :)

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Dec 31, 2019

@danbst Seems to be backwards compatible, I wouldn't be against that.

lib/modules.nix Outdated Show resolved Hide resolved
@raboof raboof requested a review from Infinisil Jan 4, 2020
@Infinisil
Copy link
Member

@Infinisil Infinisil commented Jan 4, 2020

Cool, now all that's left is to squash the commits into one, with a commit message prefix like "lib/modules: ..."

@raboof raboof force-pushed the raboof:clarifyErrorMessage branch from a05b9a8 to 8a9d5fc Jan 5, 2020
If I understand correctly, the problem isn't so much that you're assigning to
that top-level attribute, but that the assignment to the attribute (or any
child of the attribute) introduces the 'config' object and prevents 'lifting'
all settings to a generated 'config' object.
@raboof raboof force-pushed the raboof:clarifyErrorMessage branch from 8a9d5fc to 43ef3a8 Jan 5, 2020
@raboof
Copy link
Contributor Author

@raboof raboof commented Jan 8, 2020

now all that's left is to squash the commits into one, with a commit message prefix

Done, thanks!

@Infinisil Infinisil merged commit b46776d into NixOS:master Jan 8, 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
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

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