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

Python 3 support #336

Open
jpmens opened this issue Sep 19, 2018 · 29 comments
Open

Python 3 support #336

jpmens opened this issue Sep 19, 2018 · 29 comments

Comments

@jpmens
Copy link
Collaborator

jpmens commented Sep 19, 2018

Karl rightly asks whether we intend to support Python 3. The only valid response to the question is "yes".

@amotl
Copy link
Member

amotl commented Sep 19, 2018

Thanks for asking, @jpmens and Karl. I would like to extend that answer to "definitively yes" ;]. Support for Python3

@dlangille
Copy link
Contributor

Python 2.7 is EOL in about 6 weeks. Any news?

@amotl
Copy link
Member

amotl commented Nov 18, 2019

Dear Dan,

thanks for asking.

While [1] has been left behind due to my failure to continue working on that appropriately (sorry @jpmens!), I am still willing to catch up on that and finally update the codebase to support Python 3.

Comparing the branches [2] shows it should be doable without too much hassle as I believe most of the improvements landed in the plugin area. The test harness added through 257d0b8 will definitively help here.

With kind regards,
Andreas.

[1] https://github.com/jpmens/mqttwarn/tree/develop
[2] develop...master

@amotl
Copy link
Member

amotl commented Nov 18, 2019

Comparing the branches [2] shows bringing "develop" up to speed with "master" should be doable.

I've just quickly scanned the commits and identified these to be relevant to mqttwarn core, in chronological order.

Merging all the enhancements to the mqttwarn plugins should be straightforward. Thanks to all who contributed!

As I didn't follow the development thoroughly, I am humbly asking @jpmens to review this list and maybe acknowledge my findings. If I failed on that summary, I will be happy to learn otherwise.

[2] develop...master

@jpmens
Copy link
Collaborator Author

jpmens commented Nov 19, 2019

@dlangille first of all thank you for the reminder and rest assured: mqttwarn will not simply just die on that day in January, and neither will Python2. Admittedly in terms of Python2 support ....

@amotl I'm really glad to see you back! Welcome home! :-)

Both of you: unbelievable, but I dreamed of mqttwarn and Python3 last night.

I'll review @amotl's question later in the course of the afternoon (TZ=CET) as I'm giving an Ansible training which begins in 45m.

@jpmens
Copy link
Collaborator Author

jpmens commented Nov 19, 2019

@amotl I couldn't resist. Yes, you're right, and as far as I can tell we're good to go.

As soon as you have something, please go ahead and commit it. You're the boss here today (and tomorrow, etc. :-) !

@dlangille
Copy link
Contributor

@jpmens Thank you. This all came to mind over the weekend as I was replacing Python 2.7 with Python 3.6

@jpmens
Copy link
Collaborator Author

jpmens commented Nov 19, 2019

@dlangille due to my current interest in FreeBSD, mqttwarn will also be a first-class citizen on that platform. Together with your packaging expertise on that platform I expect no less than greatness. :-)

@amotl
Copy link
Member

amotl commented Nov 20, 2019

I've just updated the "develop" branch by integrating the changes from "master". It turned out to be a total no-brainer. git is amazing.

For the records, these are the conflicts I got:

offgrid:mqttwarn amo$ git checkout develop
offgrid:mqttwarn amo$ git merge master
CONFLICT (rename/delete): services/nma.py deleted in HEAD and renamed to mqttwarn/services/nma.py in develop. Version develop of mqttwarn/services/nma.py left in tree.
CONFLICT (file location): services/mysql_remap.py added in HEAD inside a directory that was renamed in develop, suggesting it should perhaps be moved to mqttwarn/services/mysql_remap.py.
CONFLICT (file location): services/alexa-notify-me.py added in HEAD inside a directory that was renamed in develop, suggesting it should perhaps be moved to mqttwarn/services/alexa-notify-me.py.
CONFLICT (modify/delete): mqttwarn.py deleted in develop and modified in HEAD. Version HEAD of mqttwarn.py left in tree.
Auto-merging README.md
Auto-merging .gitignore
CONFLICT (content): Merge conflict in .gitignore
Automatic merge failed; fix conflicts and then commit the result.

Resolving them was easy based on the review above and quickly looking up that the nma plugin indeed has been abandoned. As mqttwarn core (the former mqttwarn.py main script) did not receive any updates, I just ignored it (aka. git rm mqttwarn.py) when resolving the commit.

Also, the work-in-progress test harness (#332) has been integrated and as expected, all tests pass successfully. Anyone who is interested can also run them by just typing make test within the projects' root directory.

@amotl
Copy link
Member

amotl commented Nov 20, 2019

On the journey of making mqttwarn compatible with Python 3, I just went ahead after giving the "develop" branch some more love and published it to the Python Package Index (PyPI).

Read (and discuss) all about it within #127.

This is a huge leap forward. However, please note the codebase has not been touched to support Python 3 yet.

@amotl
Copy link
Member

amotl commented Dec 1, 2019

Hi there,

40fa69a finally brings mqttwarn core way closer to Python3.

Within #127 (comment), @dlangille offered to send a merge request for bringing all service plugins up to speed. This would be very welcome. Please make sure you are submitting it to the "develop" branch of this repository. Thanks already!

With kind regards,
Andreas.

@amotl
Copy link
Member

amotl commented Dec 16, 2019

Some service modules are still based on urllib and urllib2. urllib is the hardest module to use from Python 2/3 compatible code, see also https://python-future.org/compatible_idioms.html#urllib-module.

The list of relevant service plugins are:

In order to have the code base compatible with Python2/3, we might want to either introduce the requests module or to use http://python-future.org/.

@jpmens
Copy link
Collaborator Author

jpmens commented Dec 17, 2019

Let's take this to requests. There's hardly anything requests cannot do, and it is easy to use.

mqttwarn modules which are not converted by their maintainers will likely fall wayside.

@dgomes
Copy link
Contributor

dgomes commented Dec 17, 2019

instapush has been closed, so that one can be removed altogether

jpmens added a commit that referenced this issue Dec 17, 2019
@jpmens
Copy link
Collaborator Author

jpmens commented Dec 17, 2019

@dgomes thank you and thank you for that contribution. :-)

jpmens added a commit that referenced this issue Dec 17, 2019
@amotl
Copy link
Member

amotl commented Dec 17, 2019

Thanks already, @dgomes and @jpmens. I've further added 17bf274 and 578ab70 to address more aspects with respect to Python2/3 compatibility.

@amotl
Copy link
Member

amotl commented Dec 17, 2019

Hi there,

the latest and greatest mqttwarn-0.13.2 has just been released [1]. It should be reasonably compatible with Python 3, but we will be happy to hear about anything which doesn't work properly yet -- either on Python 2 and Python 3!

In order to mitigate eventual fallout coming from this process, we want to encourage everyone to install mqttwarn from PyPI, test it using their favorite plugins and report the outcome back to us.

Thanks already!

With kind regards,
Andreas.

[1] https://pypi.org/project/mqttwarn/

@amotl
Copy link
Member

amotl commented Dec 17, 2019

For verifying Python 3.8 support, we might want to check if things related to Jinja2 are still working in this environment, see also pallets/jinja#1119.

@amotl
Copy link
Member

amotl commented Jan 9, 2020

I just added tox support for Python 3.8 and it also works flawlessly.

I would like to close this issue if I could at least get some encouraging words from @jpmens, @ckrey, @dlangille, @rgitzel, @Gulaschcowboy or maybe others who are using mqttwarn on a regular basis.

If you are able to catch some time, I will be happy to hear on which systems you are running mqttwarn successfully these days. We might want to add this to the documentation at some place.

@jpmens
Copy link
Collaborator Author

jpmens commented Jan 11, 2020

@amotl I can pretty much not get anything working on macOS (Python 3.7.2), neither with python setup.py install nor with pip install mqttwarn, after creating a virtual environment with python -mvenv v3; source v3/bin/activate.

A trivial “hello” to topic hello/1 and output to file:f01 doesn’t work. I’ve spent a bit debugging into the file service, and the service seems to crash out when append_newline is checked in config{}. If I comment out those lines completely, I then get

mqttwarn.services.file   ] Cannot write to file `b'/tmp/f.01'': [Errno 2] No such file or directory: "b'/tmp/f.01'"    mqttwarn.services.file   ] Cannot write to file `b'/tmp/f.01'': [Errno 2] No such file or directory: "b'/tmp/f.01'"    

which makes me think something general is happening with encoding.

I’ll continue testing tomorrow.

@amotl
Copy link
Member

amotl commented Jan 12, 2020

@jpmens Thanks for your quick response and your observations. Getting things right here is very important.

something general is happening with encoding

True. I hope 0.13.7 fixes these issues through 442a51c, now also supported by appropriate software tests.

@jpmens
Copy link
Collaborator Author

jpmens commented Jan 12, 2020

@amotl that fixed it, thanks

@jpmens
Copy link
Collaborator Author

jpmens commented Jan 12, 2020

I have tested a few mqttwarn services on macOS 10.15 (Catalina)with Python 3.7.2 and on OpenBSD 6.6-CURRENT with Python 3.7.6. I chose services I'm most familiar with, log, file, smtp, pushover, and all work as expected.

You've done a fabulous job, @amotl, and I am now closing this issue.

@amotl
Copy link
Member

amotl commented Jan 12, 2020

Thanks for testing!

@rgitzel
Copy link
Contributor

rgitzel commented Jan 13, 2020

@amotl Apologies first for not having kept up. But I was glad to see all the activity. :-)

I just gave the latest code in master a go, and made what seemed to be the simplest changes to the Dockerfile to get it to run: master...rgitzel:try-python3-version

It runs, but fails miserably. The log is full of errors like this:

2020-01-13 05:22:45,962 WARNING  [mqttwarn.context         ] Cannot invoke alldata function wifi_values defined in wifi: Missing parentheses in call to 'print'. Did you mean print(message)? (funcs.py, line 179)

So presumably I need to get my functions running in Python 3 as well?

Has there been any migration documentation prepared? I'm wondering what else I'll need to change.

@jpmens
Copy link
Collaborator Author

jpmens commented Jan 13, 2020

@rgitzel that error message appears to indicate that your funcs.py file isn't Python3 compatible. It is quite likely that you will be able to get that going quickly by surrounding print statements with parentheses

print(something)

@rgitzel
Copy link
Contributor

rgitzel commented Jan 14, 2020

Upgrading my functions to Python 3 seems to have done the trick.

So, I'm seeing three times the log entries:

2020-01-14 05:06:46,521 INFO     [mqttwarn.core            ] Invoking service plugin for `log'
2020-01-14 05:06:46,521 INFO     [mqttwarn.services.log    ] wifi,Device=tasmota-garage,Router=EE:9F:DB:F1:B6:08 Signal=100 1578978406000000000
2020-01-14 05:06:46,522 INFO     [mqttwarn.core            ] Invoking service plugin for `mqttpub'
2020-01-14 05:06:46,539 INFO     [mqttwarn.core            ] Invoking service plugin for `log'
2020-01-14 05:06:46,539 INFO     [mqttwarn.services.log    ] temperature,Device=tasmota-garage,Sensor=DHT22 Temperature=-3.0 1578978406000000000
2020-01-14 05:06:46,540 INFO     [mqttwarn.core            ] Invoking service plugin for `mqttpub'
2020-01-14 05:06:46,560 INFO     [mqttwarn.core            ] Invoking service plugin for `log'
2020-01-14 05:06:46,561 INFO     [mqttwarn.services.log    ] humidity,Device=tasmota-garage,Sensor=DHT22 Humidity=95.9 1578978406000000000
2020-01-14 05:06:46,562 INFO     [mqttwarn.core            ] Invoking service plugin for `mqttpub'

Should the "Invoking..." messages be at INFO? They seem more like DEBUG.

@amotl
Copy link
Member

amotl commented May 23, 2023

Hi again,

GH-676 revealed that not all service plugins are compatible with Python 3 yet. On this matter, I just discovered the python3 branch by @SpotlightKid, which may have a few significant improvements in this area, see 1.

Would you still be up for upstreaming corresponding improvements to the service plugins selectively, Christopher?

With kind regards,
Andreas.

Footnotes

  1. https://github.com/mqtt-tools/mqttwarn/compare/main...SpotlightKid:mqttwarn:python3

@amotl amotl reopened this May 23, 2023
@SpotlightKid
Copy link

Phew, that is more than 4 years ago. I almost forgot that I did that. I certainly forgot what I did.

I'm afraid I currently don't have much time for FLOSS development and the little time I have I'd like to spend on my own projects. So I am not able to incorporate these change sino the current state of the project, sorry.

However, feel free to pick whatever you want from my branch, be it whole commits or just individual changes or bits of code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants