8000 Upgrade zeroconf to 0.27.1 by bdraco · Pull Request #36277 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Upgrade zeroconf to 0.27 8000 .1 #36277

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Jun 5, 2020
Merged

Upgrade zeroconf to 0.27.1 #36277

merged 10 commits into from
Jun 5, 2020

Conversation

bdraco
Copy link
Member
@bdraco bdraco commented May 29, 2020 •
edited
Loading

Add zeroconf as an explicit dependency to integrations that load it or load it in their deps. This ensures that zeroconf is updated without needing to restart twice.

Should also help https://community.home-assistant.io/t/python3-high-cpu-usage/160012/24

Changes

Fix for chromecast audio python-zeroconf/python-zeroconf#245

Other commits are minor sans the removal of the legacy address compat
python-zeroconf/python-zeroconf@ab72aa8
python-zeroconf/python-zeroconf@87a0fe2
python-zeroconf/python-zeroconf@488ee1e
python-zeroconf/python-zeroconf@178cec7
python-zeroconf/python-zeroconf@d881aba
python-zeroconf/python-zeroconf@beff998
python-zeroconf/python-zeroconf@10065b9

Remaining

Breaking change

There is a backwards incompatible change in 0.27
python-zeroconf/python-zeroconf@ab72aa8

The address argument has been removed, and the addresses must be used instead. Its been marked as deprecated for a while but we didn't noticed until it broke 🙉

Example of the change needed
ikalchev/HAP-python#263

Discussion:
ikalchev/HAP-python#262

cc @emontnemery in case this causes problems for cast

Proposed change

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@bdraco bdraco added the waiting-for-upstream We're waiting for a change upstream label May 29, 2020
@probot-home-assistant
Copy link

Hey there @robbiet480, @Kane610, mind taking a look at this pull request as its been labeled with a integration (zeroconf) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@bdraco bdraco assigned bdraco and unassigned robbiet480 and Kane610 May 30, 2020
@emontnemery
Copy link
Contributor
emontnemery commented May 31, 2020 •
edited
Loading

@bdraco Thanks for the heads up!
This indeed affects cast too.
pychromecast PR here: home-assistant-libs/pychromecast#368, once merged+released cast needs to bump the pychromecast version.

@basnijholt basnijholt mentioned this pull request Jun 3, 2020
Closed
@emontnemery emontnemery mentioned this pull request Jun 4, 2020
Merged
20 tasks
@bdraco bdraco changed the title Upgrade zeroconf to 0.27 Upgrade zeroconf to 0.27.1 Jun 4, 2020
@bdraco bdraco removed the waiting-for-upstream We're waiting for a change upstream label Jun 5, 2020
@bdraco bdraco marked this pull request as ready for review June 5, 2020 12:21
@bdraco
Copy link
Member Author
bdraco commented Jun 5, 2020

Verified the spurious warning is fixed in 0.27.1

@bdraco bdraco linked an issue Jun 5, 2020 that may be closed by this pull request
zeroconf updates require two restarts because the pypi package is loaded before the zeroconf integration #36476
Closed
@bdraco bdraco added this to the 0.111.0 milestone Jun 5, 2020
@bdraco
Copy link
Member Author
bdraco commented Jun 5, 2020 •
edited
Loading

So dyson, soundtouch, aiohomekit, miio, pyvizio, all load zeroconf as well

so this needs to go in setup.py instead I think but that seems like we should not do in beta.

balloob
balloob reviewed Jun 5, 2020
View reviewed changes
balloob
balloob reviewed Jun 5, 2020
View reviewed changes
@balloob
Copy link
Member
balloob commented Jun 5, 2020

Are these all integrations that use zeroconf discovery? Should we update the validation.

We explicitly allow referencing zeroconf without having it in after_deps: https://github.com/home-assistant/core/blob/dev/script/hassfest/dependencies.py#L163-L166

@balloob
Copy link
Member
balloob commented Jun 5, 2020

Actually, that hassfest check is accompanied with automatic dependency management:

When we install requirements, we actually automatically mark zeroconf as a dependency for integrations that are discoverable via zeroconf to make sure we have the latest requirements: https://github.com/home-assistant/core/blob/dev/homeassistant/requirements.py#L87-L93

@bdraco
Copy link
Member Author
bdraco commented Jun 5, 2020 •
edited
Loading

Are these all integrations that use zeroconf discovery? Should we update the validation.

Its all the integrations that import zeroconf that could load it before its updated. Many of them don't actually use zeroconf directly because of how they are integrated, but it doesn't stop them from importing.

@balloob
Copy link
Member
balloob commented Jun 5, 2020 •
edited
Loading

So hassfest should check for this.

The logic in hassfest is:

  • gather all dependencies that we import from
  • mark dependencies as allowed if discovery specified in manifest

The setup logic is:

  1. gather all requirements of integration
  2. gather all dependencies. If discovery specified in manifest, gather those integrations too.
    • For each dep: do step 1 (recursively)

So that means that all integrations that are updated in this PR that already have zeroconf in their manifest should have already had zeroconf requirements be processed.

@bdraco
Copy link
Member Author
bdraco commented Jun 5, 2020

I'm a bit confused here. For clarity, all of these integration are loading the python zeroconf pypi package via a dependency, not directly.

@balloob
Copy link
Member
balloob commented Jun 5, 2020

Oh snap, I misunderstood. Okay my bad.

@balloob
Copy link
Member
balloob commented Jun 5, 2020 •
edited
Loading

I thought that the problem was that the latest zeroconf wasn't installed. That should not happen as long as they depend on zeroconf integration via deps, after_deps or use zeroconf discovery.

@balloob
Copy link
Member
balloob commented Jun 5, 2020 •
edited
Loading

Ok, so here is why this fixes things:

The ssdp integration does not rely on zeroconf discovery. It only imports PyPI package netdisco, which imports PyPI package python-zeroconf. This would be the old version.

Now we install python-zeroconf as part of setting up the zeroconf integration, but Python will already have loaded the old python-zeroconf because it was already imported. It would require a restart to resolve imports to the new python-zeroconf.

@balloob
Copy link
Member
balloob commented Jun 5, 2020 •
edited
Loading
< 6D40 table class="d-block user-select-contain" data-paste-markdown-skip>

Restarting 3.7 test. Test failed without clear reason.

@balloob balloob merged commit 5e65d8d into home-assistant:dev Jun 5, 2020
balloob pushed a commit that referenced this pull request Jun 5, 2020
@bdraco @balloob
Upgrade zeroconf to 0.27.1 (#36277)
ca511cb
@bdraco bdraco deleted the zeroconf_027 branch February 25, 2024 00:20
bdraco added a commit that referenced this pull request Feb 25, 2024
@bdraco
Remove zeroconf from ssdp after deps
6233e1a 
This was added in #36277 but is no longer needed since
we setup discovery integrations ahead of time to ensure
their deps are updated before other integrations can load
them
@bdraco bdraco mentioned this pull request Feb 25, 2024
Merged
20 tasks
balloob pushed a commit that referenced this pull request Feb 27, 2024
@bdraco
Remove zeroconf from ssdp after deps (#111326)
587854f 
* Remove zeroconf from ssdp after deps

This was added in #36277 but is no longer needed since
we setup discovery integrations ahead of time to ensure
their deps are updated before other integrations can load
them

* adjust test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@balloob balloob balloob approved these changes

Assignees

@bdraco bdraco

Labels
cherry-picked cla-signed core dependency-bump integration: zeroconf small-pr PRs with less than 30 lines.
Projects
None yet
Milestone
0.111.0
Development

Successfully merging this pull request may close these issues.

zeroconf updates require two restarts because the pypi package is loaded before the zeroconf integration
6 participants
@bdraco @emontnemery @balloob @robbiet480 @homeassistant @Kane610
0