Conversation
f1be9c4 to
f02b748
Compare
|
I have modified the derivation to use an older version of |
f02b748 to
de0f855
Compare
|
@FRidh Could you please have a look at whether I implemented |
de0f855 to
353b3dd
Compare
FRidh
left a comment
There was a problem hiding this comment.
Looks good. I haven't reviewed the module though.
There was a problem hiding this comment.
if yarl is without name, then name can be dropped here as well.
There was a problem hiding this comment.
aiohttp still had a name attribute. I've removed it now.
There was a problem hiding this comment.
having this inside packageOverrides and then self.callPackage is I think a bit nicer, but it is not necessary.
There was a problem hiding this comment.
It also has the advantage of being able to modify the frontend package.
There was a problem hiding this comment.
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?
pkgs/top-level/all-packages.nix
Outdated
There was a problem hiding this comment.
This should simply be callPackage because we do not have any Python packages to supply as arguments.
There was a problem hiding this comment.
That's true, but it will hopefully change in the future when home-assistant allows newer versions of the dependencies :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I know, but I hope that home-assistant will just work with the standard python package set in the future.
There was a problem hiding this comment.
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.
Provide
Exactly as intended! 👍 |
021e921 to
78baeca
Compare
|
Providing |
0ac8c82 to
7471f2e
Compare
|
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. |
|
It might still take me one or two days to give this a spin, but could you provide a nixos test for it, and maybe add a way to configure it using the module directly too, using `toYAML`?
|
|
Do you have an idea of what exactly to test? I haven't yet worked with home-assistant that much. |
|
Maybe simply asserting something is listening on the web interface port and replying with a 2xx code?
|
7471f2e to
a92fddf
Compare
|
@flokli I have added a |
a92fddf to
bcfbad8
Compare
|
Finally, a NixOS test has been added. It checks whether
|
|
Btw, should I add the test to |
|
And finally, I think the package should actually go into |
bcfbad8 to
98b6e17
Compare
6b0396c to
96445cc
Compare
|
Removed from @GrahamcOfBorg build home-assistant |
GrahamcOfBorg
left a comment
There was a problem hiding this comment.
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
GrahamcOfBorg
left a comment
There was a problem hiding this comment.
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
GrahamcOfBorg
left a comment
There was a problem hiding this comment.
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
FRidh
left a comment
There was a problem hiding this comment.
The Python expressions look good to me. Just the small callPackage change is needed. I have not reviewed the module.
pkgs/top-level/all-packages.nix
Outdated
There was a problem hiding this comment.
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.
96445cc to
cfc242b
Compare
|
Now I don't use |
04e3220 to
6e6f5c6
Compare
6e6f5c6 to
0604c07
Compare
|
@GrahamcOfBorg build home-assistant @FRidh Please merge :) |
|
Success on x86_64-linux (full log) Partial log (click to expand)
|
|
Thanks! |
|
Success on x86_64-linux (full log) Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Partial log (click to expand)
|
| pname = "homeassistant"; | ||
| version = "0.62.1"; | ||
|
|
||
| diabled = !isPy3k; |
There was a problem hiding this comment.
You're right. I'll remove it with the next update.
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
build-use-sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)/cc @FRidh @f-breidenstein @flokli