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

home-assistant: init at 0.62.1 #34188

Merged
merged 7 commits into from Feb 1, 2018
Merged

home-assistant: init at 0.62.1 #34188

merged 7 commits into from Feb 1, 2018

Conversation

dotlambda
Copy link
Member

closes #33675

Regarding --skip-pip:
When I disable that option, not even the frontend component can be loaded because home-assistant isn't able to import pip. I have no idea why.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

/cc @FRidh @f-breidenstein @flokli

@dotlambda
Copy link
Member Author

I have modified the derivation to use an older version of yarl, since yarl-1.0.0 removed the quote and unquote functions used by home-assistant: https://github.com/aio-libs/yarl/blob/master/CHANGES.rst#100-2018-01-15.

@dotlambda
Copy link
Member Author

@FRidh Could you please have a look at whether I implemented extraPackages exactly as you intended it to be?

Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

Looks good. I haven't reviewed the module though.

buildPythonPackage rec {
pname = "astral";
version = "1.4";
name = "${pname}-${version}";
Copy link
Member

Choose a reason for hiding this comment

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

no name

python = python3.override {
packageOverrides = self: super: {
yarl = super.yarl.overridePythonAttrs (oldAttrs: rec {
name = "${oldAttrs.pname}-${version}";
Copy link
Member

Choose a reason for hiding this comment

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

if yarl is without name, then name can be dropped here as well.

};
});
aiohttp = super.aiohttp.overridePythonAttrs (oldAttrs: rec {
name = "${oldAttrs.pname}-${version}";
Copy link
Member

Choose a reason for hiding this comment

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

same here

Copy link
Member Author

Choose a reason for hiding this comment

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

aiohttp still had a name attribute. I've removed it now.

});
};
};
hass-frontend = python.pkgs.callPackage ./frontend.nix { };
Copy link
Member

Choose a reason for hiding this comment

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

having this inside packageOverrides and then self.callPackage is I think a bit nicer, but it is not necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

It also has the advantage of being able to modify the frontend package.

requests pyyaml pytz pip jinja2 voluptuous typing aiohttp yarl async-timeout chardet astral certifi
# From running hass
hass-frontend sqlalchemy netdisco xmltodict user-agents
] ++ extraPackages python.pkgs;
Copy link
Member

Choose a reason for hiding this comment

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

I suggest having extraPackages python.pkgs either in the let expression assigned to something, or have parentheses here around it, just for clarity. I don't know what is better. What do you think?


makeWrapperArgs = [ "--add-flags --skip-pip" ];

doCheck = false;
Copy link
Member

Choose a reason for hiding this comment

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

comment with reason?

@@ -11954,6 +11954,8 @@ with pkgs;

hiawatha = callPackage ../servers/http/hiawatha {};

home-assistant = python3Packages.callPackage ../servers/home-assistant { };
Copy link
Member

Choose a reason for hiding this comment

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

This should simply be callPackage because we do not have any Python packages to supply as arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true, but it will hopefully change in the future when home-assistant allows newer versions of the dependencies :)

Copy link
Member

Choose a reason for hiding this comment

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

If we have packages from python-packages.nix and packages defined locally (read: the referred file) then we need to update the Python package set with packageOverrides. There will not be any use for defining Python packages as parameters, and as such no case for using pythonPackages.callPackage.

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 know, but I hope that home-assistant will just work with the standard python package set in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Currently the not overridden Python package set (see arguments python and isPy3k) is mixed with the overridden set. That should not happen.

I know, but I hope that home-assistant will just work with the standard python package set in the future.

Very unlikely. We have a lot of Python applications, and for the larger ones we typically need to provide overrides.

@FRidh
Copy link
Member

FRidh commented Jan 23, 2018

Regarding --skip-pip:
When I disable that option, not even the frontend component can be loaded because home-assistant isn't able to import pip. I have no idea why.

Provide pip conditionally?

Could you please have a look at whether I implemented extraPackages exactly as you intended it to be?

Exactly as intended! 👍

@FRidh FRidh mentioned this pull request Jan 23, 2018
12 tasks
@dotlambda dotlambda force-pushed the home-assistant branch 3 times, most recently from 021e921 to 78baeca Compare January 23, 2018 16:45
@dotlambda
Copy link
Member Author

Providing pip conditionally won't work because home-assistant will still check whether it is running inside a virtualenv. I've previously tried removing https://github.com/home-assistant/home-assistant/blob/master/homeassistant/util/package.py#L10 but some home-assistant component seems to import running_under_virtualenv.

@dotlambda dotlambda force-pushed the home-assistant branch 2 times, most recently from 0ac8c82 to 7471f2e Compare January 23, 2018 17:22
@dotlambda
Copy link
Member Author

It would be nice if someone else could also test the NixOS module. The only problem I've noticed so far that the map doesn't show up on http://localhost:8123/map.

@flokli
Copy link
Contributor

flokli commented Jan 23, 2018 via email

@dotlambda
Copy link
Member Author

Do you have an idea of what exactly to test? I haven't yet worked with home-assistant that much.

@flokli
Copy link
Contributor

flokli commented Jan 23, 2018 via email

@dotlambda
Copy link
Member Author

dotlambda commented Jan 25, 2018

@flokli I have added a config option that allows specifying the configuration.yaml as a Nix attribute set. However, I wasn't sure how to deal with the case of an existing configuration.yaml file. I decided to simply delete the old one in this case.
See https://github.com/dotlambda/nixpkgs/blob/home-assistant/nixos/modules/services/misc/home-assistant.nix#L58.

@dotlambda
Copy link
Member Author

Finally, a NixOS test has been added. It checks whether

  • the config option works as expected and
  • web interface and API are accessible.

@dotlambda
Copy link
Member Author

Btw, should I add the test to nixos/release.nix?

@dotlambda
Copy link
Member Author

And finally, I think the package should actually go into python-packages.nix since it also provides a Python API: https://dev-docs.home-assistant.io/en/dev/

@dotlambda
Copy link
Member Author

Removed from python-packages.nix because this should be a consistent set of packages.
Also updated to 0.62.1.
@FRidh Is there anything left to do?

@GrahamcOfBorg build home-assistant
@GrahamcOfBorg test home-assistant

@dotlambda dotlambda changed the title home-assistant: init at 0.62.0 home-assistant: init at 0.62.1 Jan 30, 2018
Copy link

@GrahamcOfBorg GrahamcOfBorg left a comment

Choose a reason for hiding this comment

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

Success on x86_64-linux (full log)

Partial log (click to expand)

tests/components/test_init.py ............                               [ 72%]
tests/components/test_introduction.py .                                  [ 72%]
tests/components/test_logger.py ....                                     [ 75%]
tests/components/test_script.py .....                                    [ 78%]
tests/components/test_shell_command.py ......                            [ 82%]
tests/components/test_system_log.py ..............                       [ 91%]
tests/components/test_websocket_api.py ..............                    [100%]

========================= 159 passed in 10.68 seconds ==========================
/nix/store/h15jk268357qxdif5kfkjklih96641pa-homeassistant-0.62.1

Copy link

@GrahamcOfBorg GrahamcOfBorg left a comment

Choose a reason for hiding this comment

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

Success on x86_64-linux (full log)

Partial log (click to expand)

hass: exit status 1
syncing
hass: running command: sync
hass: exit status 0
test script finished in 238.19s
cleaning up
killing hass (pid 596)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/tmp/nix-build-vm-test-run-home-assistant.drv-0/vde1.ctl': Directory not empty
/nix/store/2crgxdwq1adi8hl09g52hl30v65gxv34-vm-test-run-home-assistant

Copy link

@GrahamcOfBorg GrahamcOfBorg left a comment

Choose a reason for hiding this comment

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

Success on aarch64-linux (full log)

Partial log (click to expand)

tests/components/test_init.py ............                               [ 72%]
tests/components/test_introduction.py .                                  [ 72%]
tests/components/test_logger.py ....                                     [ 75%]
tests/components/test_script.py .....                                    [ 78%]
tests/components/test_shell_command.py ......                            [ 82%]
tests/components/test_system_log.py ..............                       [ 91%]
tests/components/test_websocket_api.py ..............                    [100%]

========================= 159 passed in 31.29 seconds ==========================
/nix/store/64s228galv96j14dl4xbnmagqbydizvz-homeassistant-0.62.1

Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

The Python expressions look good to me. Just the small callPackage change is needed. I have not reviewed the module.

@@ -11954,6 +11954,8 @@ with pkgs;

hiawatha = callPackage ../servers/http/hiawatha {};

home-assistant = python3Packages.callPackage ../servers/home-assistant { };
Copy link
Member

Choose a reason for hiding this comment

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

Currently the not overridden Python package set (see arguments python and isPy3k) is mixed with the overridden set. That should not happen.

I know, but I hope that home-assistant will just work with the standard python package set in the future.

Very unlikely. We have a lot of Python applications, and for the larger ones we typically need to provide overrides.

@dotlambda
Copy link
Member Author

Now I don't use python3Packages.callPackage anymore.

@dotlambda dotlambda force-pushed the home-assistant branch 2 times, most recently from 04e3220 to 6e6f5c6 Compare January 31, 2018 03:59
@dotlambda
Copy link
Member Author

@GrahamcOfBorg build home-assistant
@GrahamcOfBorg test home-assistant

@FRidh Please merge :)
I will then create another PR with a suggestion for computing the necessary component dependencies from the config in the next days.

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Partial log (click to expand)

/nix/store/h15jk268357qxdif5kfkjklih96641pa-homeassistant-0.62.1

@FRidh FRidh merged commit d30735f into NixOS:master Feb 1, 2018
@FRidh
Copy link
Member

FRidh commented Feb 1, 2018

Thanks!

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Partial log (click to expand)

hass: exit status 1
syncing
hass: running command: sync
hass: exit status 0
test script finished in 47.32s
cleaning up
killing hass (pid 593)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/tmp/nix-build-vm-test-run-home-assistant.drv-0/vde1.ctl': Directory not empty
/nix/store/h8v8n6kaj88p98fxg5xdfxk7aqxng93l-vm-test-run-home-assistant

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Partial log (click to expand)

tests/components/test_init.py ............                               [ 72%]
tests/components/test_introduction.py .                                  [ 72%]
tests/components/test_logger.py ....                                     [ 75%]
tests/components/test_script.py .....                                    [ 78%]
tests/components/test_shell_command.py ......                            [ 82%]
tests/components/test_system_log.py ..............                       [ 91%]
tests/components/test_websocket_api.py ..............                    [100%]

========================= 159 passed in 31.11 seconds ==========================
/nix/store/64s228galv96j14dl4xbnmagqbydizvz-homeassistant-0.62.1

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Partial log (click to expand)

hass: exit status 1
syncing
hass: running command: sync
hass: exit status 0
test script finished in 58.74s
cleaning up
killing hass (pid 627)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/b3hy82vi2xn51jsrp2dd1yj6q9rbnl3g-vm-test-run-home-assistant

@dotlambda dotlambda deleted the home-assistant branch February 1, 2018 08:49
pname = "homeassistant";
version = "0.62.1";

diabled = !isPy3k;
Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot to remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. I'll remove it with the next update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants