Skip to content

Upgrade zeroconf to 0.27.1#36277

Merged
balloob merged 10 commits into
home-assistant:devfrom
bdraco:zeroconf_027
Jun 5, 2020
Merged

Upgrade zeroconf to 0.27.1#36277
balloob merged 10 commits into
home-assistant:devfrom
bdraco:zeroconf_027

Conversation

@bdraco
Copy link
Copy Markdown
Member

@bdraco bdraco commented May 29, 2020

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 probot-home-assistant Bot added core integration: zeroconf small-pr PRs with less than 30 lines. dependency-bump Pull requests that update a dependency file labels May 29, 2020
@probot-home-assistant
Copy link
Copy Markdown

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
Copy Markdown
Contributor

emontnemery commented May 31, 2020

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

@emontnemery emontnemery mentioned this pull request Jun 4, 2020
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
Copy Markdown
Member Author

bdraco commented Jun 5, 2020

Verified the spurious warning is fixed in 0.27.1

@bdraco bdraco added this to the 0.111.0 milestone Jun 5, 2020
@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Jun 5, 2020

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.

Comment thread homeassistant/package_constraints.txt
Comment thread homeassistant/components/xiaomi_miio/manifest.json Outdated
@balloob
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

bdraco commented Jun 5, 2020

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
Copy Markdown
Member

balloob commented Jun 5, 2020

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
Copy Markdown
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
Copy Markdown
Member

balloob commented Jun 5, 2020

Oh snap, I misunderstood. Okay my bad.

@balloob
Copy link
Copy Markdown
Member

balloob commented Jun 5, 2020

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
Copy Markdown
Member

balloob commented Jun 5, 2020

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
Copy Markdown
Member

balloob commented Jun 5, 2020

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 bdraco deleted the zeroconf_027 branch February 25, 2024 00:20
bdraco added a commit that referenced this pull request Feb 25, 2024
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
20 tasks
balloob pushed a commit that referenced this pull request Feb 27, 2024
* 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

Labels

cherry-picked cla-signed core dependency-bump Pull requests that update a dependency file integration: zeroconf small-pr PRs with less than 30 lines.

Projects

None yet

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