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

[Fix #244] Fix incompatibilities with python3 #273

Closed
wants to merge 7 commits into from

Conversation

szobov
Copy link
Contributor

@szobov szobov commented Jul 16, 2020

❗ :ATTENTION: in this changes I've dropped python2.* compatibility,

And I also checked only with OctoPrint 1.4.0 and Python 3.6.9, so I'd appropriate if someone could test it.

Closes #244

Thanks.

@szobov szobov mentioned this pull request Jul 16, 2020
@szobov
Copy link
Contributor Author

szobov commented Jul 16, 2020

@rlogiacco @MarcusWolschon folk, do you know someone, who can review this.

@raylas
Copy link

raylas commented Jul 17, 2020

@szobov I have this successfully working with OctoPrint 1.4.0 and Python 3.6.8.

@szobov
Copy link
Contributor Author

szobov commented Jul 22, 2020

bump

@goncalossilva
Copy link

This doesn't seem to be working on Octoprint 1.4.0 + Python 3.7.3.

Error:

2020-07-25 17:59:05,403 - py.warnings - WARNING - /home/pi/OctoPrint/venv/lib/python3.7/site-packages/octoprint_telegram/__init__.py:6: ExtDeprecationWarning: Importing flask.ext.babel is deprecated, use flask_babel instead.
  from flask.ext.babel import gettext

2020-07-25 17:59:05,404 - py.warnings - WARNING - /home/pi/OctoPrint/venv/lib/python3.7/site-packages/octoprint_telegram/__init__.py:7: ExtDeprecationWarning: Importing flask.ext.login is deprecated, use flask_login instead.
  from flask.ext.login import current_user

2020-07-25 17:59:05,418 - octoprint.plugin.core - ERROR - Error loading plugin telegram
Traceback (most recent call last):
  File "/home/pi/OctoPrint/venv/lib/python3.7/site-packages/octoprint/plugin/core.py", line 967, in _import_plugin
    module = _load_module(module_name, spec)
  File "/home/pi/OctoPrint/venv/lib/python3.7/site-packages/octoprint/plugin/core.py", line 71, in _load_module
    return imp.load_module(name, f, filename, details)
  File "/home/pi/OctoPrint/venv/lib/python3.7/site-packages/octoprint/vendor/imp.py", line 241, in load_module
    return load_package(name, filename)
  File "/home/pi/OctoPrint/venv/lib/python3.7/site-packages/octoprint/vendor/imp.py", line 215, in load_package
    return _load(spec)
  File "<frozen importlib._bootstrap>", line 696, in _load
  File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 728, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/home/pi/OctoPrint/venv/lib/python3.7/site-packages/octoprint_telegram/__init__.py", line 8, in <module>
    from .telegramCommands import TCMD # telegramCommands.
  File "/home/pi/OctoPrint/venv/lib/python3.7/site-packages/octoprint_telegram/telegramCommands.py", line 369
    p = sarge.run(myCmd, stderr=sarge.Capture(), shell=True, async=False)
                                                                 ^
SyntaxError: invalid syntax

I installed this through an archive the master branch of fork.

@szobov
Copy link
Contributor Author

szobov commented Jul 25, 2020

Hi @goncalossilva
Thank you a lot for feedback.
The problem is in dependency and described here: https://bitbucket.org/vinay.sajip/sarge/issues/38/python-37-breakage
I'll try to fix it and will update this PR (not master).

ps; really don't like how the folk from python foundation deal with compatibility. :(

@szobov
Copy link
Contributor Author

szobov commented Jul 25, 2020

@goncalossilva i've just updated PR.
Please, check.

I've also pushed changes to the fork's master, so you can try just reinstall.
Thanks a lot for your help and time. I appreciate it!

@szobov
Copy link
Contributor Author

szobov commented Jul 25, 2020

@giloser may I ask you to review this changes, please

@goncalossilva
Copy link

Thanks @szobov! Confirmed working on Octoprint 1.4.0 + Python 3.7.3.

@szobov
Copy link
Contributor Author

szobov commented Jul 26, 2020

You know, folk, it seems like this project is abandoned. :
I don't see any requests merged during last several months. Maybe it's better to change the main repo on someone's fork.

@ctgreybeard
Copy link

@szobov It does seem to be abandoned. Are you volunteering your fork? ;-)

@szobov
Copy link
Contributor Author

szobov commented Jul 26, 2020

@ctgreybeard I don't really like to create to simultaneously living independent versions. I wish to hear from this project maintainers, that they don't want to maintain it anymore.
If it's so, I could try to invest my time in it, at least to merge all this hanging PRs. But it's better to do it in this repo, not in the someone's fork.

@guysoft
Copy link
Contributor

guysoft commented Aug 1, 2020

@fabianonline ?

@szobov
Copy link
Contributor Author

szobov commented Aug 6, 2020

@giloser ping

@giloser
Copy link
Collaborator

giloser commented Aug 7, 2020

Hi! sorry it take so long.
No the project is not dropped but it's still open source and depend on the free time of people.

I cannot accept this as this dropped python2.* compatibility which is not acceptable for all the people that still work on python2.
So if you want me to check this and accept it , it has to be compatible also with python2.

I know some of the developer worked on this but I don't know the status.

Sorry

@szobov
Copy link
Contributor Author

szobov commented Aug 7, 2020

@giloser thank you a lot for the replay, I appropriate it.

But what do you feel about my suggestion here:
#269 (comment)

If you don't share my idea, we can close this PR and I can just maintain my fork for people, who are ok with pyhon3 only.

Cheers.

@goncalossilva
Copy link

This is broken again on 1.4.2. It'd be nice to rebase, or at least cherry pick #270.

@szobov
Copy link
Contributor Author

szobov commented Aug 10, 2020

@goncalossilva I've fixed it, thanks for this catch.

Btw, @giloser I'll abandon this MR, because it's really unpleasant to fix all conflicts after applying 2to3 and moving on python3.

Thanks everyone, who helped with testing. I'll cherry-pick or just merge only most important changes in my fork.

Cheers

@guysoft
Copy link
Contributor

guysoft commented Aug 11, 2020

It might be worth to apply 2to3, and then fix it once to work with both python 2 and 3.

  1. check out with git
  2. Apply 2to3
  3. git diff, each block for python 3 (what is added) put in: if sys.version_info >= (3,0):
  4. Every removed code (python 2) add in an else block.
  5. Special case for print commands: Just put brackets for both

@szobov
Copy link
Contributor Author

szobov commented Aug 11, 2020

It might be worth to apply 2to3, and then fix it once to work with both python 2 and 3.

Hey @guysoft
Thanks a lot for this suggestion.
As I've mentioned I don't want to invest my time in obsolete and dangerous things like python2, sorry. And I already applied 2to3 to the whole codebase.

May I ask you to prepare this changes and maybe push it in my branch or file and send me a patch with changes which I could apply.
It shouldn't take much time to finish it, imo.

Thanks for the understanding,
Cheers

@donvito4ever
Copy link

Don't work in Octoprint 1.4.2 and Python 3.8.5
Captura de pantalla 2020-08-21 a las 18 48 27

@szobov
Copy link
Contributor Author

szobov commented Aug 22, 2020

Don't work in Octoprint 1.4.2 and Python 3.8.5
Captura de pantalla 2020-08-21 a las 18 48 27

Hey @donvito4ever
Thanks for your feedback, but are you sure you've installed plugin from this PR? I see on your screenshot Python >=2.7,<3, but in this PR it has been changed on Python >=3.
Cheers.

@donvito4ever
Copy link

donvito4ever commented Aug 22, 2020

Don't work in Octoprint 1.4.2 and Python 3.8.5
Captura de pantalla 2020-08-21 a las 18 48 27

Hey @donvito4ever
Thanks for your feedback, but are you sure you've installed plugin from this PR? I see on your screenshot Python >=2.7,<3, but in this PR it has been changed on Python >=3.
Cheers.

Yes... I install from this link:

and from this other:

Same result...

@szobov
Copy link
Contributor Author

szobov commented Aug 25, 2020

Don't work in Octoprint 1.4.2 and Python 3.8.5
Captura de pantalla 2020-08-21 a las 18 48 27

Hey @donvito4ever
Thanks for your feedback, but are you sure you've installed plugin from this PR? I see on your screenshot Python >=2.7,<3, but in this PR it has been changed on Python >=3.
Cheers.

Yes... I install from this link:

and from this other:

Same result...

Yes, you've installed from the release, not with the changes from this PR.
You can try to use my fork, where I put everything together:

python -m pip install "https://github.com/szobov/OctoPrint-Telegram/archive/master.zip"

Or you can install from the sources using this branch:

https://github.com/szobov/OctoPrint-Telegram/archive/fix-244-python3-incompat.zip

@donvito4ever
Copy link

donvito4ever commented Aug 25, 2020 via email

@RogierSchuring
Copy link

Thanks for your work @szobov
For now, I am using your fork.

@xbarents
Copy link

Thanks @szobov , your version works for me with octoprint 1.4.2 and the octorprint python3 script update.

@TheLion
Copy link

TheLion commented Sep 22, 2020

I installed from https://github.com/szobov/OctoPrint-Telegram/archive/master.zip succesfully and notifications are send, but the emoticons are not working. They show:

\U0001f680 Hello. I'm online and ready to receive your commands.

@TheLion
Copy link

TheLion commented Sep 22, 2020

I installed from https://github.com/szobov/OctoPrint-Telegram/archive/master.zip succesfully and notifications are send, but the emoticons are not working. They show:

\U0001f680 Hello. I'm online and ready to receive your commands.

Never mind. After upgrading Octoprint to Python 3, it works just fine ;-)

@szobov
Copy link
Contributor Author

szobov commented Oct 1, 2020

I will re-open it soon with python2 compat

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

Successfully merging this pull request may close these issues.

Incompatible Python 3.6
10 participants