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

nixos/home-assistant: use override before overridePythonAttrs #118719

Merged
merged 1 commit into from
Apr 8, 2021

Conversation

dotlambda
Copy link
Member

Motivation for this change

f9bd8b1#commitcomment-49202157
@sweber83 Can you check whether this fixes it?

I think you can't use override after using overridePythonAttrs. I had the same problem in #118391 (comment).
cc @FRidh

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Apr 7, 2021
@dotlambda dotlambda changed the title home-assistant: use override before overridePythonAttrs nixos/home-assistant: use override before overridePythonAttrs Apr 7, 2021
@dotlambda dotlambda force-pushed the home-assistant-tests branch from 8db4fc4 to 45fb206 Compare April 7, 2021 10:10
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Apr 7, 2021
@sweber83
Copy link
Contributor

sweber83 commented Apr 7, 2021

@dotlambda
This fixes the problem.

What I still find a bit problematic, is the case, where the user wants to override the package herself and not only passes the package to the hass module but also to environment.systemPackages for example.
In this case the tests are executed and the user can't prevent it by using overridePythonAttrs themselves because of the problem of the invocation order.
So the user has to know, that they have to use overrideAttrs with doInstallCheck = false instead of overridePythonAttrs with doCheck = false.

@FRidh
Copy link
Member

FRidh commented Apr 7, 2021

So the user has to know, that they have to use overrideAttrs with doInstallCheck = false instead of overridePythonAttrs with doCheck = false.

Known issue. See NixOS/rfcs#67 (comment) so overrides are sent to the correct function. doCheck = doInstallCheck is separate and a legacy issue.

@sweber83
Copy link
Contributor

sweber83 commented Apr 7, 2021

@FRidh
Thanks, wasn't known to me :)

@dotlambda
Copy link
Member Author

We could add a warning to the option's description.

@dotlambda dotlambda force-pushed the home-assistant-tests branch from 45fb206 to 5e0defc Compare April 8, 2021 10:31
@Mic92
Copy link
Member

Mic92 commented Apr 8, 2021

Works for me.

Copy link
Contributor

@sweber83 sweber83 left a comment

Choose a reason for hiding this comment

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

Works as intended and the warning about overridePythonAttrs should be helpful.

@dotlambda dotlambda merged commit 5522a67 into NixOS:master Apr 8, 2021
@dotlambda dotlambda deleted the home-assistant-tests branch April 8, 2021 16:19
@mweinelt
Copy link
Member

mweinelt commented Apr 9, 2021

So, this used to work before, now it breaks. Same when I use overridePythonAttrs.

    package = (pkgs.home-assistant.override {
      extraPackages = ps: with ps; [ psycopg2 ];
    }).overrideAttrs (oldAttrs: {
      patches = [
        ./patches/brightsky.patch
        ./patches/prometheus-sensor.patch
        ./patches/prometheus-event-listener-callback.patch
      ];
      doCheck = false;
    });

I now get

error: attribute 'overridePythonAttrs' missing, at /etc/nixos/nixpkgs/nixos/modules/services/misc/home-assistant.nix:58:13

How can I apply patches after this change?

@dotlambda
Copy link
Member Author

@mweinelt Sorry! I guess it makes sense to go back to your original solution using overrideAttrs.

@mweinelt
Copy link
Member

mweinelt commented Apr 9, 2021

@sweber83 What was your initial setup that failed? Did you override the packages some way yourself?

@sweber83
Copy link
Contributor

sweber83 commented Apr 9, 2021

@mweinelt
Commit f9bd8b1 broke home assistant, when no overrides where used.
When I applied overridePythonAttrs it was also broken.
As I understand it, the problem is, that one can't call override on the result of the overridePythonAttrs invocation.
The problem with your override then seems to be similar, only that it's overridePythonAttrs which is not available, after calling overrideAttrs.
Going back to the original solution really seems best.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants