-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
nixos/nix-daemon: fix registry flake type #131814
nixos/nix-daemon: fix registry flake type #131814
Conversation
@@ -458,7 +458,7 @@ in | |||
description = "The flake reference to which <option>from></option> is to be rewritten."; | |||
}; | |||
flake = mkOption { | |||
type = types.unspecified; | |||
type = types.attrs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type does not match the default or the example. What's going on with all three?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, oversight fixed in force-push. The literalExample is actually correct and refers to a hypothetical nixpkgs
input, which is a flake, hence an attrs.
Before this commit, the `flake` option was typed with `types.unspecified`. This type get's merged via [`mergeDefaultOption`](https://github.com/NixOS/nixpkgs/blob/ebb592a04c5282f316d60cd4aba066f6e5d74b65/lib/options.nix#L119-L128), which has a line ```nix else if all isFunction list then x: mergeDefaultOption loc (map (f: f x) list) ``` `lib.isFunction` detects an attrs in the shape of `{__functor = ...}` as a function and hence this line substitutes such attrs with a function (f: f x). If now, a flake input has a `__functor` as it's output, this will coerce the once attrs to a function. This breaks a lot of things later in the stack, for example a later `lib.filterAttrs seive <LAMBDA>` will fail for obious reasons. According to @infinisil, `types.unspecified` is due to deprecation. In the meantime this PR provides a specific fix for the specific problem discovered.
73e3095
to
ecae25c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good. I've written a review for the original PR which wasn't reviewed... which I kind of understand, but alas.
@roberth I'd say this warrants backporting, does it? |
Successfully created backport PR #132363 for |
There was never an intention to setup the registry with aliases, which would not avoid a network call ayhow if an input is not specified as an registry-resolvable input. At the same time, this clarification also eliniates an error that is addressed upstream via NixOS/nixpkgs#131814 --- work-around: for spurious reasons w.r.t. functor attrs in module system Without any obvious reason, the module system appears to substitute attrs that contain a `__functor` with the value of that functor.
Before this commit, the
flake
option was typed withtypes.unspecified
.This type get's merged via
mergeDefaultOption
, which has a linelib.isFunction
detects an attrs in the shape of{__functor = ...}
asa function and hence this line substitutes such attrs with a function
(
f: f x
).If now, a flake input has a
__functor
as it's output, this willcoerce the once attrs to a function. This breaks a lot of things later
in the stack, for example a later
lib.filterAttrs seive <LAMBDA>
willfail for obious reasons.
According to @infinisil,
types.unspecified
is due to deprecation. Inthe meantime this PR provides a specific fix for the specific problem
discovered.
EDIT: this needs quite some back-porting, please someone can add the respective labels?