Add Nixpkgs invocation helper modules#231940
Conversation
roberth
requested review from
alyssais,
edolstra,
fricklerhandwerk,
infinisil and
matthewbauer
as code owners
github-actions
bot
added
6.topic: nixos
roberth
force-pushed
the
nixpkgs-module-helper
branch
from
4dbad27 to
fc99ccc
Compare
ofborg
bot
added
8.has: package (new)
fricklerhandwerk
left a comment
fricklerhandwerk
left a comment
There was a problem hiding this comment.
Just a superficial review. TBH the documentation doesn't explain to me who and what this thing is for exactly, and how it's supposed to be used. I could go into option documentation if you like, once the big picture is clear.
doc/using/configuration.chapter.md
Outdated
There was a problem hiding this comment.
By "module system application" you mean things like Home Manager? I'm not sure what you mean here.
There was a problem hiding this comment.
That's what I meant.
I don't know how to say this without going on a tangent. I think going on a tangent to explain this would distract from the content. Most users will know to skip the clause or skip the paragraph.
There was a problem hiding this comment.
| For [module system](#module-system) application authors, Nixpkgs provides a reusable module for the purpose of invoking Nixpkgs. Most users should not have to be aware of this. The documentation for its options is meant to be re-exposed in the application's documentation and is therefore not repeated here. | |
| For authors of applications based on the [module system](#module-system), Nixpkgs provides a reusable module for the purpose of invoking Nixpkgs. Most users should not have to be aware of this. The documentation for its options is meant to be re-exposed in the application's documentation and is therefore not repeated here. |
I can do a documentation review pass when the implementation is done. Unsubscribing for now, ping me if needed.
Ericson2314
left a comment
Ericson2314
left a comment
There was a problem hiding this comment.
I didn't re-review the docs; I trust @fricklerhandwerk on that.
I reviewed the rest, and it looks good modulo these small few things. Excited that we are getting started on the idea behind NixOS/rfcs#78 --- the grand module deduplication!!
roberth
force-pushed
the
nixpkgs-module-helper
branch
from
fbc4997 to
f2a2474
Compare
github-actions
bot
added
the
8.has: module (update)
roberth
changed the title
roberth
force-pushed
the
nixpkgs-module-helper
branch
2 times, most recently
from
7890fa9 to
8eb5efe
Compare
roberth
force-pushed
the
nixpkgs-module-helper
branch
from
a5fcf29 to
25a0939
Compare
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).
This hack isn't needed anymore, and would be incompatible with having submodule at `nixpkgs.*`.
A feature flag warning was rightfully picked up by ofborg. This should solve it, and hopefully help out the next person who tries to render docs for an optional module.
roberth
force-pushed
the
nixpkgs-module-helper
branch
from
25a0939 to
01df1f8
Compare
|
In the outer rings of context to this PR, I have attempted abstractions in a similar problem domain spanning beyond nixpkgs here. Note that this comment has no bearing to this PR beyond situational awareness for interested parties. |
Co-authored-by: David Arnold <[email protected]>
| { | ||
| disabledModules = [ | ||
| # This replaces the traditional nixpkgs module | ||
| ../nixpkgs.nix |
There was a problem hiding this comment.
That module has other imports that would also get removed:
nixpkgs/nixos/modules/misc/nixpkgs.nix
Lines 99 to 103 in 4f51d81
| { | ||
| disabledModules = [ | ||
| # This replaces the traditional nixpkgs module | ||
| ../nixpkgs.nix |
There was a problem hiding this comment.
Why does this have to be a separate module? How about updating the ../nixpkgs.nix one, and soft deprecating the localSystem and crossSystem options.
| { | ||
| options = { | ||
| hostPlatform = mkOption { | ||
| type = types.coercedTo types.str lib.systems.elaborate types.attrs; |
There was a problem hiding this comment.
We shouldn't use types.attrs, has weird merging behavior, types.raw should be fine
| type = types.coercedTo types.str lib.systems.elaborate types.attrs; | |
| type = types.coercedTo types.str lib.systems.elaborate types.raw; |
Same for the other options.
| { allowBroken = true; allowUnfree = true; } | ||
| ''; | ||
| # FIXME: use a mergeable type, like NixOS but not a hack | ||
| type = types.unique { message = "nixpkgs/pkgs/top-level/module.nix: Merging is not supported for the Nixpkgs config option yet."; } types.attrs; |
There was a problem hiding this comment.
| type = types.unique { message = "nixpkgs/pkgs/top-level/module.nix: Merging is not supported for the Nixpkgs config option yet."; } types.attrs; | |
| type = types.unique { message = "nixpkgs/pkgs/top-level/module.nix: Merging is not supported for the Nixpkgs config option yet."; } types.raw; |
Or attrsOf raw
| if lib.mapAttrs (_: _: null) (builtins.removeAttrs inputsSet ["hostPlatform"]) | ||
| != { hostPlatform = null; } |
There was a problem hiding this comment.
Seems wrong, but could also be done with builtins.removeAttrs inputsSet [ "hostPlatform" ] != { }
| isCross = config.buildPlatform != config.hostPlatform; | ||
| systemArgs = |
There was a problem hiding this comment.
Let's have a comment here mentioning that this is a bit of a hack and should be moved into the pkgs entrypoint function definition
| # key are usable with _memoize implementations that are not aware of them, | ||
| # as long as these additions are unused. | ||
| inputsSet = lib.filterAttrs | ||
| (_: v: v != v.isDefined && v.highestPrio < optDefaultPrio) |
There was a problem hiding this comment.
This condition needs a bit of work, v != v.isDefined seems wrong (detected by @roberth himself!)
| pkgsMemoizer = { pkgs, inputsSet, ... }: | ||
| let system = lib.systems.toLosslessStringMaybe inputsSet.hostPlatform.value.system; | ||
| in | ||
| if lib.attrNames inputsSet == [ "hostPlatform" ] && system != null |
| forAllSystems = lib.genAttrs lib.systems.flakeExposed; | ||
|
|
||
| pkgsMemoizer = { pkgs, inputsSet, ... }: | ||
| let system = lib.systems.toLosslessStringMaybe inputsSet.hostPlatform.value.system; |
There was a problem hiding this comment.
| let system = lib.systems.toLosslessStringMaybe inputsSet.hostPlatform.value.system; | |
| let system = lib.systems.toLosslessStringMaybe inputsSet.hostPlatform.value.system; |
Calling this on system sounds wrong 😆
wegank
added
2.status: stale
stale
bot
removed
the
2.status: stale
wegank
added
the
2.status: stale
6 participants
Description of changes
This makes a number of well tested and documented additions that lead up to
... using the supporting additions and improvements:
lib.systems.equalsfunction for comparing elaborated system strings.lib.systems.toLosslessStringMaybewhich is useful for converting elaborated systems to optional attribute names.lib.systemstests are now run by ofborg.This PR contains commits that have been split into a separate PR
lib.systems.equals#237512Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)