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: pythonPackages.zigpy-znp: init at 0.2.2; pythonPackages.zigpy: 0.22.2 -> 0.26.0; pythonPackages.zigpy-cc: 0.5.1 -> 0.5.2; pythonPackages.zigpy-xbee: 0.12.1 -> 0.13.0; pythonPackages.zigpy-zigate: 0.6.1 -> 0.6.2 #101557

Merged
merged 6 commits into from Oct 27, 2020

Conversation

mvnetbiz
Copy link
Contributor

Motivation for this change

Home Assistant update broke ZHA component by adding a new zigpy module zigpy-znp.

Things done

Init package pythonPackages.zigpy-znp.
This requires updating zigpy, and the other zigpy python modules.

I have checked to make sure this works on my H-A server.

  • 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.

@mvnetbiz mvnetbiz requested a review from etu October 24, 2020 14:44
@mvnetbiz mvnetbiz changed the title home-assistant: pythonPackages.zigpy-znp: init at 0.2.2 & update other zigpy-* modules home-assistant: pythonPackages.zigpy-znp: init at 0.2.2; pythonPackages.zigpy: 0.22.2 -> 0.26.0; pythonPackages.zigpy-cc: 0.5.1 -> 0.5.2; pythonPackages.zigpy-xbee: 0.12.1 -> 0.13.0; pythonPackages.zigpy-zigate: 0.6.1 -> 0.6.2 Oct 24, 2020
@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 101557 1

16 package built:
  • python37Packages.bellows
  • python37Packages.zha-quirks
  • python37Packages.zigpy
  • python37Packages.zigpy-cc
  • python37Packages.zigpy-deconz
  • python37Packages.zigpy-xbee
  • python37Packages.zigpy-zigate
  • python37Packages.zigpy-znp
  • python38Packages.bellows
  • python38Packages.zha-quirks
  • python38Packages.zigpy
  • python38Packages.zigpy-cc
  • python38Packages.zigpy-deconz
  • python38Packages.zigpy-xbee
  • python38Packages.zigpy-zigate
  • python38Packages.zigpy-znp

Copy link
Member

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

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

I noticed a few components were loosing support during the last upgrade and didn't investigate. I'm sorry that broke your setup, I'll be more careful from now on.

You seem to introduce one spurious change in zigpy-cc. Apart from that things LGTM.

Result of nixpkgs-review pr 101557 1

16 packages built:
  • python37Packages.bellows
  • python37Packages.zha-quirks
  • python37Packages.zigpy
  • python37Packages.zigpy-cc
  • python37Packages.zigpy-deconz
  • python37Packages.zigpy-xbee
  • python37Packages.zigpy-zigate
  • python37Packages.zigpy-znp
  • python38Packages.bellows
  • python38Packages.zha-quirks
  • python38Packages.zigpy
  • python38Packages.zigpy-cc
  • python38Packages.zigpy-deconz
  • python38Packages.zigpy-xbee
  • python38Packages.zigpy-zigate
  • python38Packages.zigpy-znp

Copy link
Member

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

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

I kinda expected tests to be present in all those packages and didn't verify that before, since checkInputs were given and the build passed. But all of these packages show

----------------------------------------------------------------------
Ran 0 tests in 0.000s

and they also don't have a pythonImportsCheck set up, which is so helpful, since python code can break so easily at runtime. So my recommendation is:

  1. Try to get tests working
  2. If they are not packaged in PyPi fetch from Git instead
  3. If there are no tests or they don't work at all, set doCheck = false;
  4. Set up pythonImportsCheck instead

@mweinelt mweinelt mentioned this pull request Oct 25, 2020
10 tasks
Copy link
Contributor

@etu etu left a comment

Choose a reason for hiding this comment

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

I've tested these changes on my Home assistant server with my zigbee card. With these changes my Zigbee works again! I've kinda... ignored it failing for a bit.

But it's still worth to look into the tests mentioned by @mweinelt

Copy link
Member

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

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

Thanks, the tests are all looking to be working! I have two remaining requests:

Using pytestCheckHook is advantageous if you are using pytest, since it allows you to declaratively manage the checkPhase with disabledTests and pytestFlagsArray in the future. You also don't have to specify the checkPhase that way.

Please replace pytest with pytestCheckHook everywhere and drop all checkPhases that just call pytest.

Regarding the commit messages: These packages are python3 only, so their attribute prefix should rather be python3Packages.

❯ nix-build -A pythonPackages.zigpy
error: asynctest-0.13.0 not supported for interpreter python2.7

pkgs/development/python-modules/zigpy-cc/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/zigpy-xbee/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/zigpy-zigate/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/zigpy-znp/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/zigpy/default.nix Outdated Show resolved Hide resolved
@mvnetbiz
Copy link
Contributor Author

It appears the tests are broken for python 3.7.8, possibly due to Martiusweb/asynctest#152. Should I set doCheck false for python 3.7.8?

@ofborg ofborg bot requested a review from etu October 27, 2020 00:00
@mweinelt
Copy link
Member

mweinelt commented Oct 27, 2020 via email

@mvnetbiz
Copy link
Contributor Author

I just want to check, disable the whole package, or just the tests? I disabled the package now. If that is what you meant should I change the commit message to python38Packages?

@mweinelt
Copy link
Member

mweinelt commented Oct 27, 2020

As the package does not seem to work >3.7.8 I don't really care if we disable the package or the tests.

python3Packages are currently synonym for python38Packages, so both are fine.

@mweinelt mweinelt merged commit 0ead6f8 into NixOS:master Oct 27, 2020
@mweinelt
Copy link
Member

Thanks!

@mvnetbiz mvnetbiz deleted the zigpy-znp branch November 28, 2020 03:16
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

4 participants