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
Member

@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 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
Member Author

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

@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 January 4, 2020 11:14
@infinisil
Copy link
Member

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

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

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
@infinisil infinisil added the 6.topic: module system About NixOS module system internals label Mar 19, 2020
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

3 participants