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 will break some plugins #3454

Closed
gdombiak opened this issue Feb 16, 2020 · 4 comments
Closed

Python 3 will break some plugins #3454

gdombiak opened this issue Feb 16, 2020 · 4 comments

Comments

@gdombiak
Copy link
Contributor

@gdombiak gdombiak commented Feb 16, 2020

What were you doing?

Hi @foosel, I while back I opened a ticket about some str vs bytes issue I ran into while using CancelObject plugin with OctoPrint 1.4 with Python 3. I then closed that ticket assuming the issue was with the plugin but I now think that we have a Python 3 vs Python 2 difference that will break some plugins.

  1. Put a breakpoint in DiskFileWrapper#save and/or StreamWrapper#save
  2. Upload new file to OctoPrint
  3. Execute io.BufferedReader(self.stream()).readline() from debugger
  4. Python 3 returns bytes while Python 2 returns str
  5. This will break plugins that have a process_line hook like DisplayLayerProgress or CancelObject. See LineProcessorStream#read when process line is called. The line parameter used to be of type str with Python 2 and is now bytes with Python 3.

An example of a Plugin breaking is here: OllisGit/OctoPrint-DisplayLayerProgress#117. The hard part is that there is no exception but a semantic error. :(

What did you expect to happen?

line parameter passed in LineProcessorStream#read to be of type str so existing plugins that have a hook do not break. Otherwise each plugin will need to handle str and bytes

What happened instead?

Functionality of plugins break

Did the same happen when running OctoPrint in safe mode?

In safe mode the error is not exposed. OctoPrint without plugins also works fine but problem is waiting to unleash its fury against poor unaware plugins. :)

Version of OctoPrint

Any OctoPrint version that runs with Python 3

Operating System running OctoPrint

Python 3

Printer model & used firmware incl. version

N/A

Browser and version of browser, operating system running browser

N/A

Link to octoprint.log

N/A

Link to contents of terminal tab or serial.log

N/A

Link to contents of Javascript console in the browser

N/A

Screenshot(s)/video(s) showing the problem:

Python 2
Python 2

Python 3
Python 3

I have read the FAQ.

@GitIssueBot

This comment has been minimized.

Copy link
Collaborator

@GitIssueBot GitIssueBot commented Feb 17, 2020

This issue has been mentioned on OctoPrint Community Forum. There might be relevant details there:

https://community.octoprint.org/t/lineprocessorstream-python-3-behaviour/16240/4

@foosel

This comment has been minimized.

Copy link
Member

@foosel foosel commented Feb 18, 2020

So I took a close, long and hard look at this, and came to the conclusion that I cannot do anything about this on my end.

The thing is, the class in question implements io.RawBaseIO, which means I have to adhere to its API. Which means that it needs to operate on bytes (str in Python 2, bytes in Python 3), not unicode (unicode in Python 2, str in Python 3). If I start decoding stuff into unicode before passing it to process_line I get into all kinds of troubles when further processing the result from that inside read. I could turn it back into bytes, but that just screams trouble too...

The thing is, if you work on the file system level, you work with bytes in Python. You always have, their data type was just called str in Python 2. Client code needs to recognize that. I try to make the py2/3 compat migration as painless as possible for plugins, but that's something I cannot do for them without breaking parts of OctoPrint.

So I'm leaving this as is but added some warnings to the code which should make it into the documentation. I'll also finally look into whipping up some kind of list of common pitfalls for plugin authors to refer to for the coming months of migrations. Should have done that already but other stuff was even more urgent.

@foosel foosel closed this Feb 18, 2020
foosel added a commit that referenced this issue Feb 18, 2020
Make absolutely clear that processLine will be called with a byte & not
a unicode string.

See #3454
@gdombiak

This comment has been minimized.

Copy link
Contributor Author

@gdombiak gdombiak commented Feb 21, 2020

Thanks Gina @foosel for looking into this. Your approach makes sense to me. Having documentation providing guidance about the problem and possible solutions that plugin developers should implement is going to be helpful to many people.

Regards,
Gaston

@GitIssueBot

This comment has been minimized.

Copy link
Collaborator

@GitIssueBot GitIssueBot commented Feb 26, 2020

This issue has been mentioned on OctoPrint Community Forum. There might be relevant details there:

https://community.octoprint.org/t/new-release-candidate-1-4-0rc6/16502/1

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.