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

Update to Twisted 17.5.0 #3310

Merged
merged 2 commits into from Jun 13, 2017
Merged

Update to Twisted 17.5.0 #3310

merged 2 commits into from Jun 13, 2017

Conversation

rodrigc
Copy link
Contributor

@rodrigc rodrigc commented Jun 11, 2017

@mention-bot
Copy link

@rodrigc, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tardyp, @rutsky and @sa2ajj to be potential reviewers.

@tardyp
Copy link
Member

tardyp commented Jun 12, 2017

@rodrigc looks like the py3 tests are broken by this release :-( Would you like to look at it?

@rodrigc
Copy link
Contributor Author

rodrigc commented Jun 12, 2017

In 17.5.0 more of twisted.mail has been ported to py3. So the buildbot code which uses this module needs to be fixed.

@tardyp
Copy link
Member

tardyp commented Jun 12, 2017

@rodrigc as I want to release soon. I want this to move forward. I take the task of making this PR pass.

@tardyp
Copy link
Member

tardyp commented Jun 12, 2017

@rodrigc I finally spent the afternoon working on http://twistedmatrix.com/trac/ticket/9175 :-/

@s0undt3ch
Copy link
Contributor

I stumbled on a mail notifier issue yesterday with 17.5.0, FYI. quoteaddr is being fed a module, the email module. Downgraded and its all working now.

@codecov
Copy link

codecov bot commented Jun 13, 2017

Codecov Report

Merging #3310 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3310      +/-   ##
==========================================
- Coverage   88.23%   88.23%   -0.01%     
==========================================
  Files         322      322              
  Lines       33425    33426       +1     
==========================================
- Hits        29494    29493       -1     
- Misses       3931     3933       +2
Impacted Files Coverage Δ
master/buildbot/reporters/notifier.py 96.33% <100%> (ø) ⬆️
master/buildbot/reporters/mail.py 94.08% <100%> (-1.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d940d1...e5e9088. Read the comment docs.

@tardyp
Copy link
Member

tardyp commented Jun 13, 2017

@rodrigc updated your branch with the needed unit test fixes.
I did not check if reporter.mail is actually working e2e, we just have an integration test passing, with the last sendMail part skipped. I think it is enough for now to merge it. We still don't have officially mail reporter integrated.

let me know if this is fine for you

Copy link
Contributor Author

@rodrigc rodrigc left a comment

Choose a reason for hiding this comment

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

Go ahead and merge this.
I think there is more work done to have email fully working on Python 3 with 17.5.0, but this
is a good first step. Other than the e2e test, is there a way to actually trigger sending an email with reporter.email to test this?

Thanks for fixing this.

@tardyp
Copy link
Member

tardyp commented Jun 13, 2017

@rodrigc the best would be to run a twisted sntp server if it exists in order to create a real e2e test, but this is a bit harder. I think manually testing once is fine enough.

@tardyp tardyp merged commit e7a4136 into buildbot:master Jun 13, 2017
@rodrigc
Copy link
Contributor Author

rodrigc commented Jun 13, 2017

@s0undt3ch Can you test mail notification on Python 3 + Twisted 17.5.0 with @tardyp's latest fixes?

You can test the stuff from git by doing something like:


# Create virtual environment
python3 -m venv buildbot-venv
source buildbot-venv/bin/activate
#
#
git clone https://github.com/buildbot/buildbot buildbot_test
cd buildbot_test
pip install -e pkg
pip install -e worker
pip install -e 'master[tls,tests]'

@s0undt3ch
Copy link
Contributor

Nope:

2017-06-13 12:44:32-0600 [ESMTPSender,client] Unhandled Error                                                  
        Traceback (most recent call last):                                                                                 
          File "/home/buildbot/.pyenv/versions/2.7.13/envs/Buildbot/lib/python2.7/site-packages/twisted/python/log.py", line 103, in callWithLogger
            return callWithContext({"system": lp}, func, *args, **kw)                                                      
          File "/home/buildbot/.pyenv/versions/2.7.13/envs/Buildbot/lib/python2.7/site-packages/twisted/python/log.py", line 86, in callWithContext
            return context.call({ILogContext: newCtx}, func, *args, **kw)                                  
          File "/home/buildbot/.pyenv/versions/2.7.13/envs/Buildbot/lib/python2.7/site-packages/twisted/python/context.py", line 122, in callWithContext
            return self.currentContext().callWithContext(ctx, func, *args, **kw)                               
          File "/home/buildbot/.pyenv/versions/2.7.13/envs/Buildbot/lib/python2.7/site-packages/twisted/python/context.py", line 85, in callWithContext
            return func(*args,**kw)                                                                                 
        --- <exception caught here> ---                                                                                    
          File "/home/buildbot/.pyenv/versions/2.7.13/envs/Buildbot/lib/python2.7/site-packages/twisted/internet/posixbase.py", line 597, in _doReadOrWrite
            why = selectable.doRead()                                                                             
          File "/home/buildbot/.pyenv/versions/2.7.13/envs/Buildbot/lib/python2.7/site-packages/twisted/internet/tcp.py", line 208, in doRead
            return self._dataReceived(data)                                                                
          File "/home/buildbot/.pyenv/versions/2.7.13/envs/Buildbot/lib/python2.7/site-packages/twisted/internet/tcp.py", line 214, in _dataReceived
            rval = self.protocol.dataReceived(data)                                                          
          File "/home/buildbot/.pyenv/versions/2.7.13/envs/Buildbot/lib/python2.7/site-packages/twisted/protocols/tls.py", line 330, in dataReceived
            self._flushReceiveBIO()                                                                                 
          File "/home/buildbot/.pyenv/versions/2.7.13/envs/Buildbot/lib/python2.7/site-packages/twisted/protocols/tls.py", line 295, in _flushReceiveBIO
            ProtocolWrapper.dataReceived(self, bytes)                                                                    
          File "/home/buildbot/.pyenv/versions/2.7.13/envs/Buildbot/lib/python2.7/site-packages/twisted/protocols/policies.py", line 120, in dataReceived
            self.wrappedProtocol.dataReceived(data)                                                        
          File "/home/buildbot/.pyenv/versions/2.7.13/envs/Buildbot/lib/python2.7/site-packages/twisted/protocols/basic.py", line 571, in dataReceived
            why = self.lineReceived(line)                                                                  
          File "/home/buildbot/.pyenv/versions/2.7.13/envs/Buildbot/lib/python2.7/site-packages/twisted/mail/smtp.py", line 995, in lineReceived
            why = self._okresponse(self.code, b'\n'.join(self.resp))                                              
          File "/home/buildbot/.pyenv/versions/2.7.13/envs/Buildbot/lib/python2.7/site-packages/twisted/mail/smtp.py", line 1044, in smtpState_to
            return self.smtpState_toOrData(0, b'')
          File "/home/buildbot/.pyenv/versions/2.7.13/envs/Buildbot/lib/python2.7/site-packages/twisted/mail/smtp.py", line 1062, in smtpState_toOrData
            self.sendLine(b'RCPT TO:' + quoteaddr(self.lastAddress))
          File "/home/buildbot/.pyenv/versions/2.7.13/envs/Buildbot/lib/python2.7/site-packages/twisted/mail/smtp.py", line 179, in quoteaddr
            res = email.utils.parseaddr(addr)
          File "/home/buildbot/.pyenv/versions/2.7.13/lib/python2.7/email/utils.py", line 214, in parseaddr
            addrs = _AddressList(addr).addresslist
          File "/home/buildbot/.pyenv/versions/2.7.13/lib/python2.7/email/_parseaddr.py", line 457, in __init__
            self.addresslist = self.getaddrlist()
          File "/home/buildbot/.pyenv/versions/2.7.13/lib/python2.7/email/_parseaddr.py", line 217, in getaddrlist
            while self.pos < len(self.field):
        exceptions.TypeError: object of type 'module' has no len()

@s0undt3ch
Copy link
Contributor

The requirements were bumped to >=17.5.0 right? That will break existing email setups. It works with 17.1.0

@rodrigc
Copy link
Contributor Author

rodrigc commented Jun 14, 2017

@s0undt3ch I think I found your problem on this line in Twisted: https://github.com/twisted/twisted/blob/trunk/src/twisted/mail/smtp.py#L1899

Can you hack your local copy of Twisted 17.5.0 and change:

          toEmailFinal.append(email)

to:

          toEmailFinal.append(_email)

@s0undt3ch
Copy link
Contributor

@rodrigc yes, that fixes it.

@rodrigc
Copy link
Contributor Author

rodrigc commented Jun 15, 2017

Thanks @s0undt3ch !
@tardyp how did this slip through the cracks for buildbot, since you mentioned that this bug in Twisted was introduced 10 months ago: twisted/twisted@e73c9c5

@rodrigc rodrigc deleted the twisted-17.5 branch June 15, 2017 17:49
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.

None yet

4 participants