Skip to content

flake/lib.nixosSystem: add _file-keys for error-location if needed#129924

Merged
Ma27 merged 1 commit intoNixOS:masterfrom
Ma27:flake-modules-pos
Jul 14, 2021
Merged

flake/lib.nixosSystem: add _file-keys for error-location if needed#129924
Ma27 merged 1 commit intoNixOS:masterfrom
Ma27:flake-modules-pos

Conversation

@Ma27
Copy link
Member

@Ma27 Ma27 commented Jul 11, 2021

Motivation for this change

⚠️ Caution: this is a late-night idea and I'm not sure if it's a good one! Before anybody considers to merge this, a review from our module-system experts is needed :)


When inlining a module with a problematic declaration, you usually get
get a not-so helpful error like this:

$ cat flake.nix
{
  description = "A very basic flake";
  inputs.nixpkgs.url = path:../.;
  outputs = { self, nixpkgs }: {
    nixosConfigurations.foo = nixpkgs.lib.nixosSystem {
      system = "x86_64-linux";
      modules = [
        ({ lib, ... }: { services.wrong = 2; })
        { services.nginx.enable = true; }
      ];
    };
  };
}
$ nixos-rebuild build --flake .#foo -L
error: The option `services.wrong' does not exist. Definition values:
       - In `<unknown-file>': 2

While it's certainly possible to guess where this comes from, this is
IMHO fairly confusing for beginners (and kinda reminds me of the
infamous "infinite recursion at undefined position"-error).

The module-system determines the position of a declaration using the
_file-key: this is either toString path if path is e.g. a value
from imports = [ ./foo.nix ] or the file used as NIXOS_CONFIG in
<nixpkgs/nixos>.

However such a mechanism doesn't exist (yet) for inlined flake modules,
so I tried to implement this in a fairly basic way:

  • modules = [ ./path.nix ] in lib.nixosSystem doesn't require any
    additional steps, ./path.nix is used as position for all
    declarations inside ./path.nix.

  • For non-path declarations, the position of modules inside the
    flake.nix which declares these modules is determined by doing
    unsafeGetAttrPos on the modules-argument of lib.nixosSystem.

    So the flake.nix from above would now raise the following
    error-message:

      $ nixos-rebuild build --flake .#foo -L
      error: The option `services.wrong' does not exist. Definition values:
             - In `/nix/store/4vi3nhqjyma73ygs4f93q38qjkhkaxw8-source/flake.nix': 2
    
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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 Relase notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@Ma27 Ma27 added the 6.topic: module system About "NixOS" module system internals label Jul 11, 2021
@Ma27 Ma27 requested review from cole-h, edolstra and infinisil July 11, 2021 10:34
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Jul 11, 2021
@roberth
Copy link
Member

roberth commented Jul 11, 2021

Did you try unsafeGetAttrPos on nixosSystem's args.modules?

@Ma27
Copy link
Member Author

Ma27 commented Jul 11, 2021

Good point! Gonna try it out, this should simplify this by a lot!

@Ma27 Ma27 force-pushed the flake-modules-pos branch from 8c4d757 to 50cf9b7 Compare July 11, 2021 14:15
@Ma27
Copy link
Member Author

Ma27 commented Jul 11, 2021

@roberth thanks a lot, very good point! Updated the PR accordingly! :)

@Ma27 Ma27 requested a review from infinisil July 12, 2021 21:10
@infinisil
Copy link
Member

Haven't tested it myself, but I think this should be fine.

@roberth
Copy link
Member

roberth commented Jul 14, 2021

I've added a tiny refactor to fix the formatting and add an inline doc.

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Can be merged afaic.

When inlining a module with a problematic declaration, you usually get
get a not-so helpful error like this:

    $ cat flake.nix
    {
      description = "A very basic flake";
      inputs.nixpkgs.url = path:../.;
      outputs = { self, nixpkgs }: {
        nixosConfigurations.foo = nixpkgs.lib.nixosSystem {
          system = "x86_64-linux";
          modules = [
            ({ lib, ... }: { services.wrong = 2; })
            { services.nginx.enable = true; }
          ];
        };
      };
    }
    $ nixos-rebuild build --flake .#foo -L
    error: The option `services.wrong' does not exist. Definition values:
           - In `<unknown-file>': 2

While it's certainly possible to guess where this comes from, this is
IMHO fairly confusing for beginners (and kinda reminds me of the
infamous "infinite recursion at undefined position"-error).

The module-system determines the position of a declaration using the
`_file`-key: this is either `toString path` if `path` is e.g. a value
from `imports = [ ./foo.nix ]` or the file used as `NIXOS_CONFIG` in
`<nixpkgs/nixos>`.

However such a mechanism doesn't exist (yet) for inlined flake modules,
so I tried to implement this in a fairly basic way:

* For non-path declarations, the position of `modules` inside the
  `flake.nix` which declares these modules is determined by doing
  `unsafeGetAttrPos` on the `modules`-argument of `lib.nixosSystem`.

  So the `flake.nix` from above would now raise the following
  error-message:

        $ nixos-rebuild build --flake .#foo -L
        error: The option `services.wrong' does not exist. Definition values:
               - In `/nix/store/4vi3nhqjyma73ygs4f93q38qjkhkaxw8-source/flake.nix': 2

Co-authored-by: Cole Helbling <cole.e.helbling@outlook.com>
Co-authored-by: Silvan Mosberger <github@infinisil.com>
Co-authored-by: Robert Hensing <robert@roberthensing.nl>
@Ma27 Ma27 force-pushed the flake-modules-pos branch from 30af8ed to e14c245 Compare July 14, 2021 08:14
@Ma27
Copy link
Member Author

Ma27 commented Jul 14, 2021

Squashed this together to have a nicer commit-message and re-checked this. Would merge in a minute as several folks are fine with this in the current state :)

@Ma27 Ma27 merged commit b4c4784 into NixOS:master Jul 14, 2021
@Ma27 Ma27 deleted the flake-modules-pos branch July 14, 2021 08:29
@ncfavier
Copy link
Member

Is there a reason for having this in flake.nix rather than nixos/lib/eval-config.nix? It seems to work the same.

@Ma27
Copy link
Member Author

Ma27 commented Dec 17, 2021

I don't recall the details anymore, but I think this was a flake-specific issue. Can you reproduce the overall problem with a non-flake NixOS configuration? If that's the case, please share your code with me and if needed, we should move the change to nixos/lib/eval-config.nix indeed.

@ncfavier
Copy link
Member

ncfavier commented Dec 17, 2021

The reason I'm asking is that I'm working on a PR to move much of the logic of lib.nixosSystem out of flake.nix, so if it can be done for this it's good news.

I didn't run into the overall problem, but it's easy to imagine a situation where it would arise: basically any time eval-config.nix is invoked directly with inline modules that have errors in them.

Now I'm actually wondering if this could be moved directly to lib.evalModules... I think falling back to unsafeGetAttrPos for inline modules makes sense for any use of the module system. EDIT: #126769 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: module system About "NixOS" module system internals 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants