Opened 15 years ago
Closed 13 years ago
#2385 defect closed fixed (fixed)
twisted.words irc server does not work with gaim
Reported by: | Glyph | Owned by: | David Sturgis |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | words | Keywords: | |
Cc: | Branch: |
branches/pidgin-2385-3
branch-diff, diff-cov, branch-cov, buildbot |
|
Author: | tenth |
Description
When I attempt to connect, regardless of whether I specify a password or not, gaim hangs forever and doesn't consider me logged in.
Change History (18)
comment:1 Changed 15 years ago by
comment:2 Changed 14 years ago by
Type: | enhancement → defect |
---|
Itamar and I discovered that including the MOTD lines:
("375", ":- %(serviceName)s Message of the Day - "), ("372", ":- The message of the day is: THERE IS NO MESSAGE OF THE DAY. - "), ("376", ":End of /MOTD command.")
as part of twisted.words.service.IRCUser._welcomeMessages, immediately after the RPL_MYINFO line, is consistent with FreeNode server behavior, and is more acceptable to GAIM.
comment:3 Changed 14 years ago by
Actually, you don't even need the 372 (actual message of the day) line, just 375 (RPL_MOTDSTART) and then 376 (RPL_ENDOFMOTD). hooray.
comment:4 Changed 14 years ago by
Keywords: | easy added |
---|
Awesome, thanks for the investigation. Now it should be easy for someone to adjust IRCUser
to send these codes and make gaim/pidgin work.
comment:5 Changed 14 years ago by
Author: | → tenth |
---|---|
Branch: | → branches/pidgin-2385 |
(In [25256]) Branching to 'pidgin-2385'
comment:6 Changed 14 years ago by
Keywords: | review added; easy removed |
---|---|
Owner: | Jean-Paul Calderone deleted |
comment:7 Changed 14 years ago by
Keywords: | review removed |
---|---|
Owner: | set to David Sturgis |
Here we go:
1.
- "serviceName": self.hostname, + "serviceName": "test",
Why's this change?
2.
+ self.name = nickname;
Please remove the semi-colon.
- One flake:
twisted/words/test/test_irc_service.py:8: 'time' imported but unused
Thanks!
comment:8 Changed 13 years ago by
Keywords: | review added |
---|---|
Owner: | David Sturgis deleted |
Itamar and I addressed Therve's review comments, and also added an actual test for Logins. As part of this, we also fixed some bugs found by the test (hostname is always "undefined", fake creation time, etc.)
comment:9 Changed 13 years ago by
Owner: | set to Jean-Paul Calderone |
---|---|
Status: | new → assigned |
Looking good. A few simple things:
comment:10 Changed 13 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Jean-Paul Calderone to David Sturgis |
Status: | assigned → new |
Let me try that again.
First, some simple things:
- In
service.py
, the newserverInfo
attribute of the factory needs to be documented. In particular, all the keys (and their associated values) which it will have need documentation. - In
test_service.py
, the_assertGreeting
method's new docstring has some trailing whitespace in it; it also documentsAssertionError
as an exception which may be raised, but I think it's actuallytwisted.trial.unittest.FailTest
, or maybeself.failureException
. - Further down in that file, the new
messageType
parameter to_response
is undocumented. testNickServLogin
's docstring also has trailing whitespace. As long as you're modifying the function, you should rename it to comply with the naming convention -test_nickServLogin
.- In
test_irc_service.py
- numerous lines end in trailing whitespace
- methods should be separated by two blank lines; many are, but setUp and the first test method following it are separated by only one blank line.
- The dates in the copyright statement should be made accurate - 2009, not 2001-2008.
Next. The behavior actually being implemented is very bizarre. Now the server sends the MOTD (except there's never an MOTD so it just sends two lines about how there's not an MOTD) whenever it receives NICK
. As a separate bug, the server allows multiple NICK
s to be issued. This might be a reasonable thing to allow, but it's not being handled properly now. Please file a new ticket for dealing with that misbehavior.
So, anyway. This isn't quite the behavior of other IRC servers (at least, not freenode). Freenode won't send anything until it gets both USER
and NICK
. Then it sends the whole "greeting" sequence. Then it uses NickServ to challenge you for a password. It would probably be best to emulate this more closely.
Finally, I don't really understand a lot of the changes to service.py
. The name
attribute, at least, seems necessary to make the test pass (although it wasn't before - I didn't bother to figure out why, but I'm annoyed that it's not obvious :/). However, why are nickname
and realm
being handled differently now? realm
in particular is actually part of IUser
and should probably remain that way unless there's a good reason to change it.
comment:11 Changed 13 years ago by
Keywords: | review added |
---|---|
Owner: | David Sturgis deleted |
Itamar and I have made the suggested docstring and whitespace fixes. The new ticket #3664 addresses the MOTD ordering issue. As for nickname and realm, they were required previously, but not set until after login; This new order seems to make more sense. (For example, error messages early in the login process will now contain the correct hostname.)
comment:12 Changed 13 years ago by
Keywords: | review removed |
---|---|
Owner: | set to David Sturgis |
Conflict when merging w/ trunk.
comment:13 Changed 13 years ago by
Branch: | branches/pidgin-2385 → branches/pidgin-2385-2 |
---|
(In [26276]) Branching to 'pidgin-2385-2'
comment:14 Changed 13 years ago by
Branch: | branches/pidgin-2385-2 → branches/pidgin-2385-3 |
---|
(In [26277]) Branching to 'pidgin-2385-3'
comment:15 Changed 13 years ago by
Keywords: | review added |
---|---|
Owner: | David Sturgis deleted |
Merged forward (hopefully correctly this time!)
comment:16 Changed 13 years ago by
Keywords: | review removed |
---|---|
Owner: | set to David Sturgis |
Very minor changes:
- Since the structure of
serverInfo
is a bit silly (it's a dictionary, but it needs to have specific keys, it should really be a collection of attributes) it should be private pending some better structure that we want to publicly expose. In other words, it should start with a single underscore,_serverInfo
. - A couple of blank lines have been incorrectly (I assume accidentally?) deleted which aren't really relevant to the implementation, and should be put back in. Merged to trunk, these lines are: service.py:207 service.py:350 service.py:357 service.py:888
- There is another problem, for which there should be a separate ticket. AWAY isn't implemented on the server, so when I attempt to log in, I see this (thankfully, apparently harmless) traceback:
2009-02-22 19:26:06-0500 [IRCUser,0,127.0.0.1] Unhandled Error Traceback (most recent call last): File "/home/glyph/Projects/Twisted/trunk/twisted/python/context.py", line 37, in callWithContext return func(*args,**kw) File "/home/glyph/Projects/Twisted/trunk/twisted/internet/selectreactor.py", line 146, in _doReadOrWrite why = getattr(selectable, method)() File "/home/glyph/Projects/Twisted/trunk/twisted/internet/tcp.py", line 459, in doRead return self.protocol.dataReceived(data) File "/home/glyph/Projects/Twisted/trunk/twisted/words/protocols/irc.py", line 172, in dataReceived self.handleCommand(command, prefix, params) --- <exception caught here> --- File "/home/glyph/Projects/Twisted/trunk/twisted/words/protocols/irc.py", line 184, in handleCommand self.irc_unknown(prefix, command, params) File "/home/glyph/Projects/Twisted/trunk/twisted/words/protocols/irc.py", line 191, in irc_unknown raise NotImplementedError(command, prefix, params) exceptions.NotImplementedError: ('AWAY', '', [])
Other than that it looks good! Please fix and merge.
comment:17 Changed 13 years ago by
comment:18 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(In [26281]) Make twisted.words IRC server compatible with Pidgin.
author: itamar, tenth
reviewer: therve, exarkun, z3p, glyph
Fixes #2385
While there are other issues to be resolved with MOTD and AWAY behaviors as per #3664 and #3668 respectively, twisted.words now always sends an MOTD message, which is required for Pidgin to successfully connect to it. Also, the "nickname" and "realm" attributes are now initialized in time for them to appear in error messages during login.
I checked with ethereal. The login seems to be succeeding but the Twisted Words server doesn't send very many codes in its greeting. I suppose gaim might be waiting for something which it doesn't send before considering login complete.