-
-
Notifications
You must be signed in to change notification settings - Fork 15.5k
home-assistant: Respect nixpkgs Python package overrides instead of replacing them #104161
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
Conversation
Can you fix the eval errors of ofborg? |
bc2ac87
to
59b0d4d
Compare
Oh, my bad. Amusingly I forgot to consider and test the normal case of not having any upstream Python |
Do you have an example to help me understand what does not work currently? I don't think I entirely grasp the problem. |
@mweinelt Without this patch, any Python package overrides specified in a nixpkgs overlay won't make it into the e.g. overrides specified as per below won't be available to HA currently, but will with this patch: { ... }:
{
nixpkgs.overlays = [
(self: super: {
python3 = super.python3.override { packageOverrides = ...; };
})
];
} |
Sorry, I need @dotlambda, who came up with this construct, to review this. I understand the problem, but not the solution. |
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.
I thought about whether packageOverrides
can be removed completely. Is the scenario that you have to override something in defaultOverrides
likely to occur?
# Put packageOverrides at the start so they are applied after defaultOverrides | ||
packageOverrides = lib.foldr lib.composeExtensions (self: super: { }) ([ packageOverrides ] ++ defaultOverrides); | ||
}; | ||
py = python3.override(oa: { |
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.
This is not how override
works, is it? It doesn't expect a function, at least according to https://nixos.org/manual/nixpkgs/unstable/#sec-pkg-override.
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.
Oh, after reading lib/customisation.nix
it seems like this is possible:
Lines 67 to 93 in 8e84caf
makeOverridable = f: origArgs: | |
let | |
result = f origArgs; | |
# Creates a functor with the same arguments as f | |
copyArgs = g: lib.setFunctionArgs g (lib.functionArgs f); | |
# Changes the original arguments with (potentially a function that returns) a set of new attributes | |
overrideWith = newArgs: origArgs // (if lib.isFunction newArgs then newArgs origArgs else newArgs); | |
# Re-call the function but with different arguments | |
overrideArgs = copyArgs (newArgs: makeOverridable f (overrideWith newArgs)); | |
# Change the result of the function call by applying g to it | |
overrideResult = g: makeOverridable (copyArgs (args: g (f args))) origArgs; | |
in | |
if builtins.isAttrs result then | |
result // { | |
override = overrideArgs; | |
overrideDerivation = fdrv: overrideResult (x: overrideDerivation x fdrv); | |
${if result ? overrideAttrs then "overrideAttrs" else null} = fdrv: | |
overrideResult (x: x.overrideAttrs fdrv); | |
} | |
else if lib.isFunction result then | |
# Transform the result into a functor while propagating its arguments | |
lib.setFunctionArgs result (lib.functionArgs result) // { | |
override = overrideArgs; | |
} | |
else result; |
(pay attention to the
if
in the definition of overrideWIth
)But then I think we should use
py = python3.override(oa: { | |
py = python3.override (oldAttrs: { |
because that's common naming.
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.
Not actually oldArgs
?
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.
True, that makes even more sense. Or origArgs
, as it's called above.
}; | ||
py = python3.override(oa: { | ||
# Put packageOverrides at the start so they are applied after | ||
# defaultOverrides and upstream overrides |
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.
Maybe
# defaultOverrides and upstream overrides | |
# defaultOverrides and overlay overrides |
is a better wording?
packageOverrides = lib.foldr lib.composeExtensions (self: super: { }) ( | ||
[ packageOverrides ] | ||
++ defaultOverrides | ||
++ (lib.optional (oa ? packageOverrides) oa.packageOverrides)); |
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.
++ (lib.optional (oa ? packageOverrides) oa.packageOverrides)); | |
++ lib.optional (oa ? packageOverrides) oa.packageOverrides; |
or
++ (lib.optional (oa ? packageOverrides) oa.packageOverrides)); | |
++ oa.packageOverrides or [ ]; |
If you override |
I marked this as stale due to inactivity. → More info |
Is this still relevant? |
Doesn't apply anymore since #182112 and I wonder if it is still required thereafter. |
Motivation for this change
Currently, home-assistant's definition replaces any existing Python package overrides instead of just layering on top of them, preventing HA from using any packages in existing overrides. This changes that.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)