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: Ignore more attributes + doc #8332

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

roberth
Copy link
Member

@roberth roberth commented May 14, 2023

Motivation

Context

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

Comment on lines 660 to 671
name == "lib"
|| name == "configurations"
|| name == "darwinConfigurations"
|| name == "darwinModules"
|| name == "debug"
|| name == "flakeModule"
|| name == "flakeModules"
|| name == "herculesCI"
|| name == "homeConfigurations"
|| name == "internals"
|| name == "modules"
|| name == "nixopsConfigurations"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hold on guys, are we now adding not only nixpkgs specific code to nix but also random flakes specific code? 👎

Copy link
Member Author

Choose a reason for hiding this comment

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

configurations and modules are for #6257. I don't suppose those are a problem?

Do you mean debug and internals? I think ignoring those is useful for any flake that wants to be easy to troubleshoot.

These changes are quite intentional and not random or specific to any flake?

If you mean that we shouldn't add code specific to the flakes feature, I can agree with that. It would be nice to separate that out, but I don't think that's a particularly productive choice of component to spin off. I'd rather do that with the dev shell logic, a component whose independent development would pay off more, and where it would make sense to have pinnable or alternate implementations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I meant the old ones.
configurations and modules are fine but don't solve anything
instead of internals and debug maybe it would be nice to keep a single attribute since both of them generally are going to be used for the same thing

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the separation. For instance, internals could be used in a shebang script, whereas debug is only for human consumption and therefore can be assumed that messing around with it is ok.
Using debug from a shebang script seems like a bad idea, but debugging using internals isn't quite as bad. I suppose internals.debug could be a convention, but the indirection is unnecessary and a bit annoying when you're using it in the repl. So internals is definitely the one to keep, but debug is also worth the tiny cost.

@roberth
Copy link
Member Author

roberth commented May 16, 2023

Sharing what I said in a chat:

I think it'd be good progress though, that [this] PR
And I should note that the Nix team has previously decided that they don't want to validate modules
So there's not a lot of room for improvement in the PR anyway
I suppose it could check that modules.<class> are attrsets, but that's about it
I feel like it's also too soon to require configurations.<class>.<name> to be _type = "configuration"; that's been merged like one or two weeks ago in the module system
Also we don't know if a forcing a flat .<name> for configs is even good enough for the module system applications out there
I think home manager has an alternative take on multi system configurations, so I think it's not up to nix flake check to incentivize anything yet

roberth added a commit to hercules-ci/nixpkgs that referenced this pull request Jul 6, 2023
A helper for module system applications.

Naming compatible with NixOS/nix#6257
To be supported by nix flake check in NixOS/nix#8332
Concept is in the spirit of a proposed module based solution to [RFC 0078 System-agnostic configuration file generators](NixOS/rfcs#78).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation flakes new-cli Relating to the "nix" command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants