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

flake check: allow modules to be paths #7356

Closed
wants to merge 1 commit into from

Conversation

ncfavier
Copy link
Member

Fixes #6866

src/nix/flake.cc Outdated
if (v.type() == nPath) {
PathSet context;
auto path = state->coerceToPath(pos, v, context);
state->evalFile(path, v);
Copy link
Member

Choose a reason for hiding this comment

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

Since v is a reference pointing into the flake, evalFile will mutate the flake attribute.
That could lead to surprising behavior down the road.
You could copy v into a temporary Value after forcing v and work with that instead of v.

Copy link
Member Author

Choose a reason for hiding this comment

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

Made a pointer to avoid copying.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually that was silly, copying Values is cheap.

@edolstra
Copy link
Member

edolstra commented Nov 28, 2022

Not in favor of this. In general we should avoid magic overloads. Imagine if Nix had a type system - what would the type of nixosModules.* be? (It would have to be "a module or a file that when imported evaluates to a module".) It's also pretty ad hoc - why do only modules get this special behaviour?

Also, this seems to be entirely to work around a deficiency in the NixOS module system (namely that it uses file names as keys). It would be better to fix this in the module system.

@roberth
Copy link
Member

roberth commented Nov 28, 2022

In general we should avoid magic overloads.

Following this line of reasoning, Nix should be a compiled language, because otherwise, it'd have to use runtime type tags.
The module system, like Nix, is a DSL, which means that it takes a few liberties to make common use cases easier. Furthermore, its implementation is restricted by the primitives that are available in the Nix language.

Also, this seems to be entirely to work around a deficiency in the NixOS module system (namely that it uses file names as keys). It would be better to fix this in the module system.

The module system has good reason not to hide path imports from its implementation. Removing path imports from the module system would break everything and cause a huge regression in error message quality.

So how? I've maintained the module system for three years. I've dealt with this very part of the system. What you're suggesting is wishful thinking.

I'll be happy to see a replacement for the module system that's actually better, but that is not a project we can start before making Flakes release-worthy.

You can't change the module system by disallowing this, so please don't. You're only making the Flakes user experience worse.

NixOS has been a construct that exists entirely within the language, and that Nix did not have to be aware of. Arguably, nix flake check is overreaching by performing NixOS's work, and should instead expose a general eval-time checking facility that not just NixOS but any module implementation can use. This could be supported by flake-parts or similar. All Nix would have to do is define an attribute that contains the error messages. Lacking such a feature, and as a fallback for "manually" written flakes, Nix should check what it reasonably can, without reporting false error messages.

Copy link
Member

@Artturin Artturin left a comment

Choose a reason for hiding this comment

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

nix flake check does not work in nixpkgs without this

what @roberth said is true

@thufschmitt thufschmitt self-assigned this Dec 9, 2022
@fricklerhandwerk
Copy link
Contributor

Discussed in Nix team meeting 2022-12-09:

Agreement: Maintaining knowledge about NixOS modules is a layer violation. This is outside of Nix's responsibility.

Decision: Close the pull request.

Complete discussion
  • @fricklerhandwerk: confused that nix flake check requires Nix to know so much about flake consumers
    • @thufschmitt: agree, it's a layer violation
    • @edolstra: in the absence of a type system that's the best there is right now
    • @roberth: this is what flake-parts solves in an opt-in way and makes nix flake check redundant
      - @edolstra: alternative: remove checking for NixOS modules altogether
      - @fricklerhandwerk: would like to have a more principled approach, like inversion of control where consumers determine the output attributes
    • @edolstra: that was discussed in the past. agree in general, but that would require opening the discussion on the general design again. that should be done before stabilising flakes.

@ncfavier ncfavier deleted the flake-check-module-path branch December 12, 2022 09:57
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2022-12-09-nix-team-meeting-minutes-15/23951/1

@roberth
Copy link
Member

roberth commented Dec 12, 2022

Decision: Close the pull request.

The proposed alternative is to remove this check altogether.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

''nix flake check'' requires modules be in a form unamenable to deduplication
7 participants