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

Rebase with Upstream and Port to Python3 #7

Merged
merged 17 commits into from
Aug 11, 2020

Conversation

Saumya-Mishra9129
Copy link
Member

Twisted uses a module named 'attr' which I couldn't find in sugar . So I installed it manually using pip (apt-get didn't work) with following command-

sudo pip install attrs

Also we need to install OpenSSL with sudo apt-get install python3-openssl to get it working.

@chimosky @quozl @srevinsaju Please Review.

@srevinsaju
Copy link
Member

srevinsaju commented Jun 10, 2020

Nice; will review it now.
OMG: heavy diff

Copy link
Member

@srevinsaju srevinsaju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing is tedious, 555 files from the last two commits. Some minor corrections; Will update you with sugar testing

afk_manager.py Outdated Show resolved Hide resolved
afk_manager.py Outdated Show resolved Hide resolved
chat_box.py Outdated Show resolved Hide resolved
chat_box.py Outdated Show resolved Hide resolved
constantly/_constants.py Outdated Show resolved Hide resolved
@Saumya-Mishra9129
Copy link
Member Author

Twisted uses a module named 'attr' which I couldn't find in sugar . So I installed it manually using pip (apt-get didn't work) with following command-
sudo pip install attrs

I now found out that we don't need to do this. It will get install when we run -
sudo apt-get install python3-service-identity It is in Readme.md

@srevinsaju
Copy link
Member

srevinsaju commented Jun 10, 2020

Can we depend on distribution specific twisted and other packages? Need to consult others too.

this will probably reduce the work to be done. for example as has been done with Box2D in physics activity

@srevinsaju

This comment has been minimized.

@Saumya-Mishra9129
Copy link
Member Author

Can we depend on distribution specific twisted and other packages? Need to consult others too.

this will probably reduce the work to be done. for example as has been done with Box2D in physics activity

Yeah it is probably a good thing to be done. If others agree I will do that as well.

@Saumya-Mishra9129 are you defaultnick who is joining #sugar?

Yes I tried to join with Polari.
Got an error

Screenshot from 2020-06-10 20-05-52

@srevinsaju
Copy link
Member

srevinsaju commented Jun 10, 2020

It is currently very buggy, the UI is better than IRC activity though; I can get to the UI, but can do nothing; Getting lots of repeated errors

builtins.UnicodeDecodeError: 'utf-8' codec can't decode byte 0xe4 in position 127: invalid contin
uation byte

Its possible that some files used within is buggy or is a escape character; probably it was not encoded in the proper charset

@Saumya-Mishra9129
Copy link
Member Author

Saumya-Mishra9129 commented Jun 10, 2020

It is currently very buggy, the UI is better than IRC activity though; I can get to the UI, but can do nothing; Getting lots of repeated errors

builtins.UnicodeDecodeError: 'utf-8' codec can't decode byte 0xe4 in position 127: invalid contin
uation byte

Its possible that some files used within is buggy or is a escape character; probably it was not encoded in the proper charset

The error is in while irc.py inside twisted, seems like after port to python3 because all strings are stored as Unicode in an instance of the str type, there can be bugs in twisted.

@srevinsaju
Copy link
Member

srevinsaju commented Jun 10, 2020

Ok, I am not spamming #sugar any more;

seems like after port to python3 because unicode strings are not accepted, there is bugs in twisted.

Twisted is a good repository, and maintained. If there was an error, it should have been found out before. It seems like correct data types were not passed as parameters. It will require advance debugging however


Btw, there is a typo in PR title

@Saumya-Mishra9129 Saumya-Mishra9129 changed the title Reabse with Upstream and Port to Python3 Rebase with Upstream and Port to Python3 Jun 10, 2020
@Saumya-Mishra9129
Copy link
Member Author

Saumya-Mishra9129 commented Jun 10, 2020

Twisted is a good repository, and maintained. If there was an error, it should have been found out before. It seems like correct data types were not passed as parameters. It will require advance debugging however

Yeah possible, I agree with you , It would need debugging of source code.

@chimosky
Copy link
Member

chimosky commented Jun 10, 2020

@Saumya-Mishra9129 said

Yeah it is probably a good thing to be done. If others agree I will do that as well.

Yeah I agree, use distro specific packages where possible.

In setup.py cmd_genpot add --from-code=UTF-8 as part of your arguments to xgettext as an error is thrown by xgettext when trying to parse some strings in Python 3 if the encoding isn't specified.. See sugarlabs/sugar-toolkit-gtk3@ecf335c.

Didn't review cd8a53e...5814bac as I can't properly view the diff of the aggregate change.
I'll wait for you to use distro specific packages before reviewing.

@Saumya-Mishra9129
Copy link
Member Author

Saumya-Mishra9129 commented Jun 11, 2020

Didn't review cd8a53e...5814bac as I can't properly view the diff of the aggregate change.
I'll wait for you to use distro specific packages before reviewing.

It is done.@chimosky Please review.

builtins.UnicodeDecodeError: 'utf-8' codec can't decode byte 0xe4 in position 127: invalid contin
uation byte

I am not able to get the cause of this error. @quozl can you look once.

@quozl
Copy link
Contributor

quozl commented Jun 29, 2020

What do you need for the UnicodeDecodeError? Are all the source files UTF-8 encoded, and do they contain only UTF-8?

Fixes error seen while making connection.
while sending a line , it takes channel as an encoded object.

Unhandled Error
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/twisted/python/log.py", line 103, in callWithLogger
    return callWithContext({"system": lp}, func, *args, **kw)
  File "/usr/lib/python3/dist-packages/twisted/python/log.py", line 86, in callWithContext
    return context.call({ILogContext: newCtx}, func, *args, **kw)
  File "/usr/lib/python3/dist-packages/twisted/python/context.py", line 122, in callWithContext
    return self.currentContext().callWithContext(ctx, func, *args, **kw)
  File "/usr/lib/python3/dist-packages/twisted/python/context.py", line 85, in callWithContext
    return func(*args,**kw)
--- <exception caught here> ---
  File "/usr/lib/python3/dist-packages/twisted/internet/posixbase.py", line 614, in _doReadOrWrite
    why = selectable.doRead()
  File "/usr/lib/python3/dist-packages/twisted/internet/tcp.py", line 243, in doRead
    return self._dataReceived(data)
  File "/usr/lib/python3/dist-packages/twisted/internet/tcp.py", line 249, in _dataReceived
    rval = self.protocol.dataReceived(data)
  File "/usr/lib/python3/dist-packages/twisted/words/protocols/irc.py", line 2631, in dataReceived
    basic.LineReceiver.dataReceived(self, data)
  File "/usr/lib/python3/dist-packages/twisted/protocols/basic.py", line 572, in dataReceived
    why = self.lineReceived(line)
  File "/usr/lib/python3/dist-packages/twisted/words/protocols/irc.py", line 2637, in lineReceived
    line = line.decode("utf-8")
builtins.UnicodeDecodeError: 'utf-8' codec can't decode byte 0xe4 in position 121: invalid continuation byte

Tested with Ubuntu 20.04 and sucrose version 0.117
@Saumya-Mishra9129
Copy link
Member Author

Update - UnicodeDecodeError is fixed in d4b7c80. I have tested also works fine. @chimosky @srevinsaju @quozl Please Review.

@quozl
Copy link
Contributor

quozl commented Jul 9, 2020

Thanks.

Fixes #6

Polari upstream has changed from Python to JavaScript. But this code remains Python. When you say you rebased with upstream, what did you do?

Reviewed. Prepared a diffstat. Most of the change is to remove the embedded twisted, which is good.

Tested. Speaking on #sugar without registration gives no message. A message is sent to the client by the server but is not displayed.

@Saumya-Mishra9129
Copy link
Member Author

Saumya-Mishra9129 commented Jul 9, 2020

Polari upstream has changed from Python to JavaScript. But this code remains Python. When you say you rebased with upstream, what did you do?

By rebasing with upstream I thought of rebasing libraries used ( twisted , zope, incremental and constantly) with their upstream sources. So I first did that, but then @chimosky suggested to use distro specific twisted ,which I have done later.

Next step would be now to rebase with Polari upstream now probably??

@quozl
Copy link
Contributor

quozl commented Jul 9, 2020

I see, thanks. Our toolkit doesn't have support for JavaScript GObject Introspection apps, so rebasing Polari itself would be a major undertaking requiring new toolkit code as well. I don't think we need that yet. But you might check for changes to Polari between the time this source code was copied and before they switched to JavaScript.

That could be a separate pull request though.

@Saumya-Mishra9129
Copy link
Member Author

I see, thanks. Our toolkit doesn't have support for JavaScript GObject Introspection apps, so rebasing Polari itself would be a major undertaking requiring new toolkit code as well. I don't think we need that yet. But you might check for changes to Polari between the time this source code was copied and before they switched to JavaScript.

Thanks, for Polari source code, I couldn't find source code which was related to Python3 and also that's so old also.

That could be a separate pull request though.

Thanks I will make a different one. Until then you can merge these changes.

@quozl
Copy link
Contributor

quozl commented Aug 11, 2020

I've checked again, and I no longer think this activity has any association with the GNOME Polari app. Polari activity is an independent implementation using Twisted, and the name is coincidental.

@quozl quozl merged commit 8f1f126 into sugarlabs:master Aug 11, 2020
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.

4 participants