Bug 436568 - Review Request: supybot - Cross-platform IRC bot written in Python
Summary: Review Request: supybot - Cross-platform IRC bot written in Python
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Kevin Fenzi
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-03-07 23:27 UTC by Ricky Zhou
Modified: 2008-08-04 18:46 UTC (History)
6 users (show)

Fixed In Version: 0.83.3-6.fc9
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-06-03 07:35:06 UTC
Type: ---
Embargoed:
kevin: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
Patch to add macro for origname (case) and fix line length of some items (2.92 KB, patch)
2008-05-07 15:34 UTC, Douglas E. Warner
no flags Details | Diff

Description Ricky Zhou 2008-03-07 23:27:03 UTC
Spec URL: http://ricky.fedorapeople.org/pkgs/supybot/supybot.spec
SRPM URL: http://ricky.fedorapeople.org/pkgs/supybot/supybot-0.83.2-1.fc8.src.rpm
Description: Supybot is a robust, user-friendly, and programmer-friendly Python IRC bot.
It aims to be an adequate replacement for most existing IRC bots.  It
includes a very flexible and powerful ACL system for controlling access
to commands, as well as more than 50 builtin plugins providing around
400 actual commands.

This is my first package, so I think I need a sponsor.

Comment 1 Ricky Zhou 2008-03-07 23:52:07 UTC
Actually, I thought about it a bit, and I think that Supybot (as opposed to
supybot) might fit the recommendation of the Package Naming Guidelines better,
so here's are the corrected URLs:
Spec URL: http://ricky.fedorapeople.org/pkgs/Supybot/Supybot.spec
SRPM URL: http://ricky.fedorapeople.org/pkgs/Supybot/Supybot-0.83.2-1.fc8.src.rpm

Sorry for the inconvenience,
Ricky

Comment 2 Ricky Zhou 2008-03-08 00:14:52 UTC
Updated to use a consistent macro style and match the source URL guidelines more
precisely. 

Spec URL: http://ricky.fedorapeople.org/pkgs/Supybot/Supybot.spec
SRPM URL: http://ricky.fedorapeople.org/pkgs/Supybot/Supybot-0.83.2-2.fc8.src.rpm

Comment 3 Dennis Gilmore 2008-03-08 01:15:11 UTC
fails to build in mock for devel

error: Installed (but unpackaged) file(s) found:
   /usr/lib/python2.5/site-packages/supybot-0.83.1_darcs-py2.5.egg-info

Comment 4 Ricky Zhou 2008-03-08 02:56:29 UTC
(In reply to comment #3)
> fails to build in mock for devel
> 
> error: Installed (but unpackaged) file(s) found:
>    /usr/lib/python2.5/site-packages/supybot-0.83.1_darcs-py2.5.egg-info
Ah, thanks for catching that.  I read
http://fedoraproject.org/wiki/Packaging/Python/Eggs and followed the
instructions there to generate the egg-info.  I tested in mock with
fedora-8/fedora-devel, and it looks like it builds properly now.  

Spec URL: http://ricky.fedorapeople.org/pkgs/Supybot/Supybot.spec
SRPM URL: http://ricky.fedorapeople.org/pkgs/Supybot/Supybot-0.83.2-3.fc8.src.rpm

Comment 5 Jeffrey C. Ollie 2008-03-08 05:27:45 UTC
Is there a reason that you aren't packaging 0.83.3?  0.83.3 was released last
October while 0.83.2 is over 1 1/2 years old.

Comment 6 Ricky Zhou 2008-03-08 07:02:33 UTC
(In reply to comment #5)
> Is there a reason that you aren't packaging 0.83.3?  0.83.3 was released last
> October while 0.83.2 is over 1 1/2 years old.
Argh, careless mistake!  I was wondering why it didn't seem to be playing
nicely with Python 2.5.  (Also, the large green link at
http://sourceforge.net/project/platformdownload.php?group_id=58965 is for
0.83.2, for some reason...)

Anyway, thanks a lot for noticing this :)

Here is the updated spec file/SRPM:
Spec URL: http://ricky.fedorapeople.org/pkgs/Supybot/Supybot.spec
SRPM URL: http://ricky.fedorapeople.org/pkgs/Supybot/Supybot-0.83.3-1.fc8.src.rpm

Side question: Some of the included plugins require an ancient (years old)
version of the PySQLite module.  Am I absolutely obligated to get that into
Fedora in order to get Supybot in?  Of course, Supybot can be very useful even
if it is missing certain plugins, and this dependency is supposed to be killed
in the next release.  

Comment 7 manuel wolfshant 2008-03-09 12:04:46 UTC
No, you are not obligated.
Ditch those plugins for now and release a new version of the suppybot rpm
if/once you have additional functional stuff.

Comment 8 Ricky Zhou 2008-03-09 21:19:38 UTC
(In reply to comment #7)
> No, you are not obligated.
> Ditch those plugins for now and release a new version of the suppybot rpm
> if/once you have additional functional stuff.
Ah, thanks, that's a relief to hear :).  Do I actually need to kill the plugins
from the package?  Supybot will just gracefully fail to load those plugins,
giving an message that that PySQLite is required (I imagine that this could
possibly be useful for people that want the functionality and are willing to
install PySQLite 1.0.x themselves).

Comment 9 manuel wolfshant 2008-03-10 01:28:56 UTC
I am not at all familiar with supybot's settings, but I suggest to provide (if
possible) a default configuration file which does not try to load the missing
plugins. I say this would be nicer than printing an error message because of a
failed load attempt.

Comment 10 Ricky Zhou 2008-03-10 02:13:09 UTC
(In reply to comment #9)
> I am not at all familiar with supybot's settings, but I suggest to provide (if
> possible) a default configuration file which does not try to load the missing
> plugins. I say this would be nicer than printing an error message because of a
> failed load attempt.
I don't think this is easy possible, as Supybot's configuration files are
usually generated by supybot-wizard, a script that will ask the user what
plugins they want to enable by default.  

Comment 11 manuel wolfshant 2008-03-10 09:21:42 UTC
I've just taken a [not very attentive] look at the config of the bot we use in
our network and I think that there are 2 places where you can do modifications:
a) It has the following lines in the conf:
###

###
# Determines what plugins will be loaded.
#
# Default value:
###
supybot.plugins: Web Network Lart Ctcp Limiter Insult Note Dict Factoids
Internet Reply Config ChannelStats Status String Relay For
mat MoobotFactoids Utilities Admin User Scheduler Services News Channel
NickCapture Plugin AutoMode Anonymous ChannelLogger URL Quo
teGrabs Filter Alias Games Karma Praise Time Protector ShrinkUrl Herald Google
Math Misc Topic Nickometer Unix Quote Owner Seen Tod
o RSS


So you can ship a default /etc/sysconfig/supybot.conf / etc/default/supybot.conf
/ whatever name you have for the default (eventually generated by you with the
wizard) which does not try to load missing plugins

b) the wizard seems to enumerate the plugins, based on the directory name. So no
directory -> no plugin to be configured -> no plugin in the conf file


Comment 12 Gianluca Sforna 2008-03-11 14:08:52 UTC
(In reply to comment #1)
> Actually, I thought about it a bit, and I think that Supybot (as opposed to
> supybot) might fit the recommendation of the Package Naming Guidelines better,
> so here's are the corrected URLs:

Just wanted to add my 0.02 to this package review I was waiting for since some
time now :)

Personally, I find packages with uppercase names harder to find and manipulate.
I know there is no strict rule about the topic but I never get the correct case
at first attempt of "yum install", so I'd appreciate if you stick with the
original naming.

Other opinions welcome

Comment 13 Ricky Zhou 2008-03-11 23:01:41 UTC
(In reply to comment #11)
> I've just taken a [not very attentive] look at the config of the bot we use in
> our network and I think that there are 2 places where you can do modifications:
> a) It has the following lines in the conf:
[snip]
> So you can ship a default /etc/sysconfig/supybot.conf / etc/default/supybot.conf
> / whatever name you have for the default (eventually generated by you with the
> wizard) which does not try to load missing plugins
Most people probably use supybot-wizard to generate their configuration (this
script asks them specific information, such as bot nickname, server, and
password.  It also creates the appropriate directories for a Supybot
instance).  It might turn out to be harder for the user to have to manually do
a lot of this, including finding and hand-editing the default config.

> b) the wizard seems to enumerate the plugins, based on the directory name. So no
> directory -> no plugin to be configured -> no plugin in the conf file
I could do this, but my original justification for not doing so was so that
users that chose to install the old PySQLite would still have these plugins
available to them.  Is there any sort of packaging policy on how these kind of
"optional" dependencies should be handled?  


Comment 14 Ricky Zhou 2008-03-17 03:12:58 UTC
(In reply to comment #12)
> Personally, I find packages with uppercase names harder to find and manipulate.
> I know there is no strict rule about the topic but I never get the correct case
> at first attempt of "yum install", so I'd appreciate if you stick with the
> original naming.
I'll try to ask upstream about their opinion on this.  I'll post an update here
with their preference on the capitalization.  

Also, I updated the spec file to use macros for everything (I was told that it
was a matter of my preference, and I like macros :)).  

Spec URL: http://ricky.fedorapeople.org/pkgs/Supybot/Supybot.spec
SRPM URL: http://ricky.fedorapeople.org/pkgs/Supybot/Supybot-0.83.3-2.fc8.src.rpm

Comment 15 Ricky Zhou 2008-04-05 23:59:14 UTC
OK, couple of updates on this.  I asked Jeremy Fincher about this, and he said
that the official name is "Supybot," but lowercased "supybot" is fine as well. 
Any other opinions on this?  

Now, a bigger problem is that some of the included plugins have full python
modules packaged with the plugin itself.  Some are in Fedora, and some are not.
 A few aren't even maintained anymore, and at least one seems to be a modified
file from another application.  

I can ask upstream about removing the maintained/unmodified ones (such as
dateutil and feedparser), but I'm not quite sure what to do about the others. 
From what I've seen, Debian, openSUSE, and Gentoo seem to just leave all of
these files in, but that doesn't feel like the right thing to do.  Any ideas how
to handle this?  The full list of of these files is:

plugins/Dict/dictclient.py
plugins/Google/google.py
plugins/Google/GoogleSOAPFacade.py
plugins/Google/SOAP.py
plugins/Math/convertcore.py
plugins/RSS/feedparser.py
plugins/Time/dateutil/*

Side note: This no longer passes rpmlint since I put a %files macro in the
ChangeLog - That'll gone from the next spec file (hopefully along with the
plugins problem).

Comment 16 manuel wolfshant 2008-04-06 00:11:37 UTC
The safest bet would be to not package those plugins. On the other hand, you
could submit (or at least build) them as separate packages. Or bribe someone to
package them for you :) We have a couple of very active python maintainers
around here, maybe someone would be willing to give a hand.
But, as you have anticipated, including private copies of python modules is a
big NO NO (in my opinion at least). Maybe this would not violate the letter of
http://fedoraproject.org/wiki/Packaging/Guidelines#head-17396a3b06ec849a7c0c6fc3243673b17e5fed90
but would definitely contradict the spirit.

Comment 17 Ricky Zhou 2008-04-06 06:47:33 UTC
(In reply to comment #16)
> But, as you have anticipated, including private copies of python modules is a
> big NO NO (in my opinion at least). Maybe this would not violate the letter of
>
http://fedoraproject.org/wiki/Packaging/Guidelines#head-17396a3b06ec849a7c0c6fc3243673b17e5fed90
> but would definitely contradict the spirit.
Yeah, I definitely agree.  Here's what I've done:

plugins/Dict/dictclient.py: Submitted a review request for python-dictclient
(bug #441098).  It seems very old (2002) and I'm not sure how well-maintained it
is, but it seems useful for the Supybot plugin (and other uses). 
 
plugins/Google/*: I just deleted this plugin entirely.  Google doesn't even give
SOAP API keys anymore, so this shouldn't be a huge loss.

plugins/Math/convertcore.py: I left this one in, since it is a modified module
from a GPLed project (as mentioned at the top of the file).  I changed the
license tag and added a comment to clarify.

plugins/RSS/feedparser.py: Deleted.  This is provided by python-feedparser.

plugins/Time/dateutil/*: Deleted.  This is provided by python-dateutil.  

Once again, I'm not requiring any of those modules because they aren't
absolutely necessary for the operation of Supybot.  How do you suggest that I
document the requirements for certain plugins, though?  Should I mention these
in some sort of README.fedora or something?  

Spec URL: http://ricky.fedorapeople.org/pkgs/Supybot/Supybot.spec
SRPM URL: http://ricky.fedorapeople.org/pkgs/Supybot/Supybot-0.83.3-3.fc8.src.rpm

Comment 18 Douglas E. Warner 2008-05-05 23:09:58 UTC
From NamingGuidelines 1]:
"the maintainer should use his/her best judgement when considering how to name 
the package. While case sensitivity is not a mandatory requirement, case 
should only be used where necessary. [...] However, if they do not express any 
preference of case, you should default to lowercase naming."

I would take this (given your comment on what upstream said) that you should 
probably go w/ lowercase "supybot".

[1] http://fedoraproject.org/wiki/Packaging/NamingGuidelines

Comment 19 Ricky Zhou 2008-05-07 14:17:58 UTC
(In reply to comment #18)
> I would take this (given your comment on what upstream said) that you should 
> probably go w/ lowercase "supybot".
Sure, that sounds good.  Here's an updated package with some other fixes as
well.  (I was calling rm -rf on %{python_sitelib}/... instead of
%{buildroot}%{python_sitelib}/..., which is obviously bad.)

Now, my main question is how to handle dependencies of plugins.  I don't think
supybot should depend on packages that only one plugin requires, so how would I
tell users that they need to install a package to use a certain plugin?  Then
again, there are only three small packages that fall under this category:
python-dateutil, python-feedparser, and python-dictclient (under review, bug
#441098), so maybe it wouldn't be horrible to require them either.  Any thoughts?

Spec URL: http://ricky.fedorapeople.org/pkgs/supybot/supybot.spec
SRPM URL: http://ricky.fedorapeople.org/pkgs/supybot/supybot-0.83.3-4.fc9.src.rpm

Comment 20 Douglas E. Warner 2008-05-07 15:33:47 UTC
I was trying to figure out how to handle the dependencies as well.  
Subpackages would be the most ideal since they wouldn't pull in unneeded 
dependencies but might be a good deal of work to maintain (since you'd have to 
pull out all the plugins separately for the core package).

Another dependency we might consider is the Twisted package 
(python-twisted-core and python-twisted-names needed on F8) since it's 
necessary for SSL connections.

I'm hoping to become a sponsor soon and can help you get this (and the other 
package) through review; I'll hopefully hear tomorrow after the FESCo meeting.

Comment 21 Douglas E. Warner 2008-05-07 15:34:39 UTC
Created attachment 304773 [details]
Patch to add macro for origname (case) and fix line length of some items

Comment 22 Ricky Zhou 2008-05-07 18:58:33 UTC
Applied, thanks!  The only part that I didn't use were the %{name} substitutions
in the file list (since I don't think those were necessarily semantically
related to the name of the package.)

Spec URL: http://ricky.fedorapeople.org/pkgs/supybot/supybot.spec
SRPM URL: http://ricky.fedorapeople.org/pkgs/supybot/supybot-0.83.3-5.fc9.src.rpm

Comment 23 Kevin Fenzi 2008-05-24 04:02:21 UTC
I'd be happy to do a formal review of this and sponsor you.
Look for a full review in a bit. 

Comment 24 Kevin Fenzi 2008-05-24 05:00:23 UTC
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
See below - License
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
72f8f28f1d847b9070be1bc5f8b002a4  Supybot-0.83.3.tar.bz2
72f8f28f1d847b9070be1bc5f8b002a4  Supybot-0.83.3.tar.bz2.orig
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
OK - No rpmlint output.
OK - final provides and requires are sane.

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should have dist tag
OK - Should package latest version

Issues:

Some suggestions (non blocker):

1. You might want to add a:

Provides: Supybot = %{version}-%{release}

In case someone tries to install it via the upstream name.

2. You might comment that the patch simply removes the Google plugin
in the spec file.

3. On dependencies, it's really up to you, but some options:

- Just require all the needed packages. How much does that really pull
in? it doesn't look like it would be that much. Network and disk is cheap
these days.

- Add a README.Fedora file that explains that you can optionally install
package X for plugin Y and it will be detected at runtime.

- Split out some of the plugins that have additional requirements.
This is less than ideal, IMHO. They are all small programs and the
maint and overhead isn't worth it really.

All those are really non blockers and up to you to decide before import. 
Let me know if you want me to look over any final changes before import. 

Otherwise this package looks great and is APPROVED.
Thanks for assisting here Douglass!

Comment 25 Ricky Zhou 2008-05-24 05:47:57 UTC
Thanks for the suggestions.  For the dependencies, you're right - I downloaded
the extra dependencies and saw that they were only ~2.5 MB, so I've added them
to the spec file.  

Thanks, everybody!

Spec URL: http://ricky.fedorapeople.org/pkgs/supybot/supybot.spec
SRPM URL: http://ricky.fedorapeople.org/pkgs/supybot/supybot-0.83.3-6.fc9.src.rpm

Comment 26 Ricky Zhou 2008-05-24 06:08:43 UTC
New Package CVS Request
=======================
Package Name: supybot
Short Description: Cross-platform IRC bot written in Python
Owners: ricky
Branches: F-8 F-9
InitialCC:
Cvsextras Commits: yes

Comment 27 Juha Tuomala 2008-05-24 08:11:23 UTC
This is great news, many thanks for everyone involved!

What is the process to get this into EPEL and are there any showstoppers in 
dependency wise preventing it?

Comment 28 Kevin Fenzi 2008-05-24 18:00:17 UTC
cvs done.

Comment 29 Ricky Zhou 2008-05-24 18:16:14 UTC
(In reply to comment #27)
> What is the process to get this into EPEL and are there any showstoppers in 
> dependency wise preventing it?
It looks like all of the dependencies are already in EPEL.  I'll definitely take
some time to read through the EPEL guidelines and look at getting it in.  

Comment 30 Fedora Update System 2008-05-24 23:34:30 UTC
supybot-0.83.3-6.fc9 has been submitted as an update for Fedora 9

Comment 31 Fedora Update System 2008-05-24 23:36:04 UTC
supybot-0.83.3-6.fc8 has been submitted as an update for Fedora 8

Comment 32 Juha Tuomala 2008-05-26 06:35:09 UTC
rpm -ql supybot|grep -c etc
0

Did I miss something, or why there isn't any initscript for starting 
or configs? When I did my own rpm for this I wrote one and don't see
why the default pkg shouldn't have one. 

Imo even it's not configured ready, it should at least start as a 
process (which in this case is bit tricky) or then say something 
that would point the admin into right document to get started quickly.
Like:

Starting supybot:                                        [FAILED]
No bots configured, read instructions the /usr/share/doc/supybot/README.fedora 

Comment 33 Ricky Zhou 2008-05-27 06:08:11 UTC
(In reply to comment #32)
> Did I miss something, or why there isn't any initscript for starting 
> or configs? When I did my own rpm for this I wrote one and don't see
> why the default pkg shouldn't have one. 
I really don't think an init script is necessary for something like an IRC bot.
 I consider a program like Supybot to be a user process more rather than a
system one (as somebody installing supybot, I'd expect to have a setup where
multiple users can each use supybot-wizard and run their own separate instances).  

Comment 34 Kyle VanderBeek 2008-05-27 06:18:37 UTC
(In reply to comment #33) 
> I really don't think an init script is necessary for something like an IRC bot.
>  I consider a program like Supybot to be a user process more rather than a
> system one (as somebody installing supybot, I'd expect to have a setup where
> multiple users can each use supybot-wizard and run their own separate
instances).  

For my $0.02, I agree.  IRC bots are mostly used by users, so I don't think an
init script is appropriate or needed.

Comment 35 Juha Tuomala 2008-05-27 06:49:49 UTC
> I consider a program like Supybot to be a user process more rather than a
> system one (as somebody installing supybot, I'd expect to have a setup where
> multiple users can each use supybot-wizard and run their own 
> separate instances). 

And if not, what it would hurt to have it and not use it? With little 
effort we could please both needs, especially when the other one is 
more serious (root controls, the machine is likely to be dedicated for that).

I think there should be one. IRC and bots are nowdays used as 
tools of OSS development and considered as service.

Comment 36 manuel wolfshant 2008-05-27 11:33:07 UTC
Even if default install does not enable the bot to run system wide, I for one
would like to see a readme.Fedora containing a sample script which can be used
by users (or even copied to /etc/init.d for those who do want a system wide bot).

Comment 37 Fedora Update System 2008-05-29 02:48:33 UTC
supybot-0.83.3-6.fc9 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update supybot'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2008-4598

Comment 38 Fedora Update System 2008-06-03 07:30:40 UTC
supybot-0.83.3-6.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 39 Fedora Update System 2008-06-03 07:35:04 UTC
supybot-0.83.3-6.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 40 Ricky Zhou 2008-08-03 20:33:13 UTC
Package Change Request
======================
Package Name: supybot
New Branches: EL-4 EL-5

Comment 41 Kevin Fenzi 2008-08-04 18:46:29 UTC
cvs done.


Note You need to log in before you can comment on or make changes to this bug.