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

[Request]: Update Discovery plugin to be python3 compatible #3664

Closed
zeonin opened this issue Jul 31, 2020 · 9 comments
Closed

[Request]: Update Discovery plugin to be python3 compatible #3664

zeonin opened this issue Jul 31, 2020 · 9 comments
Labels
done Done but not yet released request Feature request
Milestone

Comments

@zeonin
Copy link

zeonin commented Jul 31, 2020

Is your feature request related to a problem? Please describe.

Right now the built-in Discovery plugin depends on pybonjour for Bonjour/Zeroconf, which is only compatible with python2. As python2 has been deprecated and OctoPrint has been moving towards python3 compatibility, it would be nice to not lose support for Zeroconf.

Describe the solution you'd like

This line in the plugin suggests either porting pybonjour to python3 or to find an alternative. Personally, I would like to see a library that has had active contributions in the past decade as opposed to porting pybonjour. A quick search has turned up python-zeroconf as a potential replacement that is actively developed.

Describe alternatives you've considered
Alternatively, pybonjour could be modernized enough to run against python3. It seems there was a previous attempt, though it doesn't seem that it's been touched in about 7 years.

Additional context
None at the moment, though I might try to give it a shot updating the plugin to python-zeroconf.

@fieldOfView
Copy link
Contributor

python-zeroconf is what is used in Cura, though Cura uses it for discovery as opposed to advertising itself (which is what OctoPrint would do). It is actively maintained and the maintainers are open to (well-tested) PRs if needed.

@foosel
Copy link
Member

foosel commented Jul 31, 2020

Looks like porting over to python-zeroconf is fairly straight forward, I just threw around 2h at it and it's already looking very shippable. Just need to test the discovery helpers.

maintenance branch/1.4.x will have to stick to python-zeroconf 0.19.1 as that's the last version to officially support Python 2 (which 1.4.x still needs to support). On devel/2.0.0 I'll use a current version.

@foosel foosel added the request Feature request label Jul 31, 2020
@foosel foosel added this to the 1.4.2 milestone Jul 31, 2020
@foosel foosel added the done Done but not yet released label Jul 31, 2020
@foosel
Copy link
Member

foosel commented Jul 31, 2020

Well, that was easy. Implemented on maintenance and ready for 1.4.2.

@fieldOfView
Copy link
Contributor

From experience with Cura, there might be some serious performance issues on certain networks with versions 0.18-0.24:
python-zeroconf/python-zeroconf#133

These were fixed here:
python-zeroconf/python-zeroconf#205, released as 0.24.1

Would you be open to a solution where Python 2 instances use pybonjour and Python 3 instances use python-zeroconf?

@foosel
Copy link
Member

foosel commented Jul 31, 2020

It looks like this only relates to browsing, which is only exposed via a helper and not used by any bundled plugins. So the risk of this causing issues is probably fairly small.

An alternative to a python version specific switch would be to vendor bundle it for 1.4.x and apply the changes from the PR to the bundled version.

@fieldOfView
Copy link
Contributor

So the risk of this causing issues is probably fairly small.

👍
Even so it is probably a good idea to ask for feedback specifically from users with many (hundreds) Apple devices on their network once 1.4.2 RC comes around. Unless you feel you need a reason to buy 100 iphones to set up testing this yourself ofcourse 😉.

@foosel
Copy link
Member

foosel commented Jul 31, 2020

So, slightly changed here. Vendored lib for Py2, adjusted by the aforementioned performance patch plus some minor equality check fixes and also a bit of backporting of some method signatures to allow one client code for both the vendored lib and the officially maintained one. The latter will be automatically used under Py3.

@foosel foosel modified the milestones: 1.4.2, 1.4.3 Aug 6, 2020
@foosel
Copy link
Member

foosel commented Aug 6, 2020

Make that 1.4.3, 1.4.2 just got turned into a hotfix release.

@foosel
Copy link
Member

foosel commented Aug 10, 2020

Actually make that 1.5.0 because I've decided to finally fully adhere to semver and maintenance releases will see a minor increase in the future instead of patch, patch will be used for hotfixes if needed.

@foosel foosel closed this as completed in 0663728 Nov 30, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
done Done but not yet released request Feature request
Projects
None yet
Development

No branches or pull requests

3 participants