Bug 208169 - Review Request: python-twisted-core - An asynchronous networking framework written in Python
Review Request: python-twisted-core - An asynchronous networking framework wr...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Paul Howarth
Fedora Package Reviews List
:
Depends On: 207265
Blocks: FE-ACCEPT 171543 poker-network 221310
  Show dependency treegraph
 
Reported: 2006-09-26 14:52 EDT by Thomas Vander Stichele
Modified: 2008-02-20 15:30 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-12-27 09:53:00 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
kevin: fedora‑cvs+


Attachments (Terms of Use)
Fixes for issues in comment #1 (6.29 KB, patch)
2006-10-30 15:45 EST, Jeffrey C. Ollie
no flags Details | Diff

  None (edit)
Description Thomas Vander Stichele 2006-09-26 14:52:22 EDT
Spec URL: http://thomas.apestaart.org/download/pkg/fedora-5-i386-extras/python-twisted-core-2.4.0-3.fc5/python-twisted-core.spec
SRPM URL: http://thomas.apestaart.org/download/pkg/fedora-5-i386-extras/python-twisted-core-2.4.0-3.fc5/python-twisted-core-2.4.0-3.fc5.src.rpm
Description: 
An extensible framework for Python programming, with special focus
on event-based network programming and multiprotocol integration.

It is expected that one day the project will expanded to the point
that the framework will seamlessly integrate with mail, web, DNS,
netnews, IRC, RDBMSs, desktop environments, and your toaster.
Comment 1 Paul Howarth 2006-09-27 12:39:01 EDT
First pass comments:

1. The files list is very long and results in lots of "File listed twice"
warnings from rpmbuild. These could be fixed by removing these lines from the
%files list:

  %{python_sitearch}/twisted/manhole/ui/*.py*
  %{python_sitearch}/twisted/manhole/ui/*.glade
  %{python_sitearch}/twisted/manhole/ui/gtkrc
  %{python_sitearch}/twisted/persisted/journal/

  However, the whole %{python_sitearch} %files tree could be simplified down to:

  %dir %{python_sitearch}/twisted/
  %{python_sitearch}/twisted/*.py*
  %{python_sitearch}/twisted/application/
  %{python_sitearch}/twisted/cred/
  %{python_sitearch}/twisted/enterprise/
  %{python_sitearch}/twisted/internet/
  %{python_sitearch}/twisted/manhole/
  %{python_sitearch}/twisted/persisted/
  %dir %{python_sitearch}/twisted/plugins/
  %{python_sitearch}/twisted/plugins/*.py*
  %ghost %{python_sitearch}/twisted/plugins/dropin.cache
  %{python_sitearch}/twisted/protocols/
  %{python_sitearch}/twisted/python/
  %{python_sitearch}/twisted/scripts/
  %{python_sitearch}/twisted/spread/
  %{python_sitearch}/twisted/tap/
  %{python_sitearch}/twisted/test/
  %{python_sitearch}/twisted/trial/

2. There is lots of unpackaged documentation in the doc/ directory. How about a
separate -doc subpackage?

3. rpmlint output:

  E: python-twisted-core non-executable-script
/usr/lib64/python2.4/site-packages/twisted/internet/glib2reactor.py 0644
  W: python-twisted-core devel-file-in-non-devel-package
/usr/lib64/python2.4/site-packages/twisted/spread/cBanana.c
  W: python-twisted-core devel-file-in-non-devel-package
/usr/lib64/python2.4/site-packages/twisted/protocols/_c_urlarg.c
  E: python-twisted-core non-executable-script
/usr/lib64/python2.4/site-packages/twisted/trial/test/scripttest.py 0644
  E: python-twisted-core script-without-shebang
/usr/share/zsh/site-functions/_twisted_zsh_stub

  The non-executable-script errors could be fixed by quick couple of seds in %prep:

  sed -i -e '/^#! *\/usr\/bin\/python/d' twisted/internet/glib2reactor.py
  sed -i -e '/^#!\/bin\/python/d'        twisted/trial/test/scripttest.py

  The script-without-shebang error could be fixed by installing
/usr/share/zsh/site-functions/_twisted_zsh_stub with mode 644

  Not sure about the devel-file-in-non-devel-package warning; are these devel
files or are they needed at runtime for something? Are they needed at all?

4. Strictly speaking the package should have a dependency on zsh for the
ownership of the %{_datadir}/zsh/site-functions directory. I guess the "right"
think to do would be to break out a separate -zsh subpackage for it, but that
seems rather like overkill for one tiny file. Thoughts?

5. I think the URL for this package should be
http://twistedmatrix.com/trac/wiki/TwistedCore, with
http://www.twistedmatrix.com/ reserved for the python-twisted metapackage.

Comment 2 Kevin Fenzi 2006-10-01 17:07:24 EDT
Paul: Are you reviewing this package? If so, this should block FE-REVIEW. 
I am going to change it to do so. If you aren't reviewing, change it back to FE-
NEW and reassign back to nobody@fedoraproject.org
Comment 3 Jeffrey C. Ollie 2006-10-29 15:28:40 EST
The python-zope-interface package has been approved (at least conditionally). 
It would be nice to move this review forward too.  Paul, are you able to review
this package soon?  If not I'd be willing to do the review.  I'd like to have
the new twisted packages available so that I can play with Elisa...
Comment 4 Paul Howarth 2006-10-30 02:52:40 EST
I was hoping that Thomas would respond to the comments I made in Comment #1
before doing a full review. Please feel free to take over the review if you
want; you may well be more familiar with the package than I am (I just use
Twisted Core and Web for the current bittorrent package).
Comment 5 Jeffrey C. Ollie 2006-10-30 15:45:47 EST
Created attachment 139771 [details]
Fixes for issues in comment #1
Comment 6 Thomas Vander Stichele 2006-11-01 08:31:51 EST
http://thomas.apestaart.org/download/pkg/fedora-5-i386-extras/python-twisted-core-2.4.0-4.fc5/

I took some of Jeffrey's changes (with credit) but not all.

- I want to keep the %{__python} macros, I adapted the spec from pyvault and I'd
prefer to keep differences reasonable.
- I want to keep the %{origname} macro
- the Spec already uses $RPM_BUILD_ROOT so I don't want to mix with %{buildroot}
- I prefer the manifest as it is because this allows me to notice new files when
I update for new source releases, and that allows me to fix the problems that
are similar to the current ones mentioned (packaging .c files, wrong execution, ...)

These updated packages do not generate any rpmlint warnings for me anymore.
Comment 7 Paul Howarth 2006-11-03 08:47:40 EST
A few more quick comments before I do a more detailed review (hopefully later
today):

1. I think the Group for the -doc subpackage should be Documentation

2. I think the Group for the -zsh subpackage should be System Environment/Shells
(à la bash-completion in Extras)

3. Is %{python_sitearch}/twisted/python/_twisted_zsh_stub needed, or can it be
removed since we have %{_datadir}/zsh/site-functions/_twisted_zsh_stub?

4. The docs in the -zsh subpackage are duplicates of the same docs in the main
package. How about as an alternative:

%prep
...
# Generate a brief README.zsh
awk '/^Zsh Notes:/,/^Have fun!/' twisted/python/zshcomp.py > README.zsh
%files zsh
...
%doc README.zsh

5. TwistedCore contains an extensive test suite. After installing the package, I
can run it OK using "trial twisted", but it seems to fail some tests when I try
to run it in the buildroot in %check; any thoughts on what to do about testing?
Comment 8 Paul Howarth 2006-11-07 10:11:21 EST
Review:

- package and spec naming OK
- package meets guidelines
- license is MIT, matches spec, text included
- spec file written in English and is legible
- sources match upstream
- package builds ok in mock for rawhide, fc5, and fc6 (i386 and x86_64)
- BR's OK
- no locales or libraries to worry about
- not relocatable
- no directory ownership or permissions problems
- %clean section present and correct
- macro usage is consistent enough
- code, not content
- large doc directory properly split off into -doc subpackage
- docs don't affect runtime
- no pkgconfig files or libtool archives to worry about
- not a GUI app, no desktop file needed
- package appears to work OK
- scriptlet is sane
- subpackages have proper dependencies

Nits:

rpmlint output:
  W: python-twisted-core strange-permission twisted-dropin-cache 0775
  W: python-twisted-core mixed-use-of-spaces-and-tabs (spaces: line 7, tab: line 39)

  Both trivially fixed; there's no need to have twisted-dropin-cache executable
  in the SRPM as it's installed with the correct mode anyway.

Also see Comment #7.

Once these are addressed, I'll be happy to approve.
Comment 9 Thomas Vander Stichele 2006-11-07 11:35:30 EST
Thanks for the comments.  New version:
http://thomas.apestaart.org/download/pkg/fedora-5-i386-extras/python-twisted-core-2.4.0-5.fc5/python-twisted-core-2.4.0-5.fc5.src.rpm

I don't know what the strange-permission warning is about.  I changed to 0755
and it did the same.  Pretty crappy warning in any case, how should one know
what to do about it ?

Anyway, let me know if this version satisfies you to the point of approval :)

Thanks
Comment 10 Jeffrey C. Ollie 2006-11-07 11:42:15 EST
(In reply to comment #9)
>
> I don't know what the strange-permission warning is about.  I changed to 0755
> and it did the same.  Pretty crappy warning in any case, how should one know
> what to do about it ?

I think that it wants the source files to be 0644.
Comment 11 Thomas Vander Stichele 2006-11-07 11:45:30 EST
What ? That's silly!  It's a script, it should be executable, I want to be able
to test and execute it.  Is this an important thing to fix ?
Comment 12 Jeffrey C. Ollie 2006-11-07 12:09:13 EST
(In reply to comment #11)
> What ? That's silly!  It's a script, it should be executable, I want to be able
> to test and execute it.  Is this an important thing to fix ?

That's rpmlint for ya... Anyway, the permission of the file in the SRPM doesn't
matter since you use "install -m 755" to install it during the build.
Comment 13 Paul Howarth 2006-11-08 06:31:19 EST
OK, I'm happy with this now.

As Jeffrey says in Comment #12, there's no need for the script to be executable
in the SRPM, but that's not a blocker.

If you can figure out at some point how to get the test suite to run from the
buildroot in %check, it would be good to add that.

Approved.

Will you be submitting the other Twisted components for review now?
Comment 14 Jeffrey C. Ollie 2006-11-14 23:11:20 EST
Thomas, ping!  I'm sure you must be busy but is there a chance that you could
import this soon?
Comment 15 Thomas Vander Stichele 2006-11-15 07:13:02 EST
Well, it's imported and I'm currently up to step 10 in the process.

However I"m stumped by this page:
http://www.fedoraproject.org/wiki/Extras/CVSSyncNeeded

the page says I HAVE to provide a bugzilla number without mentioning what the
bug in question should be about.  Should I create a bug to ask for branches,
then edit this wiki ? Should I link to this bug ? Is there some magic creating a
bug for me when I import into cvs ?

Please let me know so I can do what I need to do and clarify the wiki.
Comment 16 Paul Howarth 2006-11-15 07:17:46 EST
(In reply to comment #15)
> I"m stumped by this page:
> http://www.fedoraproject.org/wiki/Extras/CVSSyncNeeded
> 
> the page says I HAVE to provide a bugzilla number without mentioning what the
> bug in question should be about.  Should I create a bug to ask for branches,
> then edit this wiki ? Should I link to this bug ? Is there some magic creating a
> bug for me when I import into cvs ?
> 
> Please let me know so I can do what I need to do and clarify the wiki.

The bugzilla number is the one for the review ticket, which would be 208169 in
this case; no need to create a new one.
Comment 17 Gianluca Sforna 2006-11-15 10:40:54 EST
(In reply to comment #15)
> 
> Please let me know so I can do what I need to do and clarify the wiki.
> 

I took this opportunity to make my first "contribution" to the wiki... feel free
to rephrase the comment if you still feel it is not clear.
Comment 18 Thomas Vander Stichele 2006-11-15 15:23:09 EST
ok, that helps.  Did that step.
Comment 19 Thomas Vander Stichele 2006-11-17 05:05:22 EST
so, poll time.  Should I already request builds for FC6 and devel, or should we
first move on with the rest of the twisted stack until we have a
feature-complete replacement for the FC-5 complete python-twisted package ?

Creating tickets for the next packages
Comment 20 Paul Howarth 2006-11-17 05:47:39 EST
I think we should review, import, and build all of the packages in a bottom-up
dependency order. So the python-twisted metapackage would be the last one, and
that is the only one that has been present in previous releases, so that's the
only one that should cause an issue for anyone, so long as nobody builds any
other package requiring one of the new python-twisted-* packages before the
top-level metapackage is available.
Comment 21 Jeffrey C. Ollie 2006-11-17 10:02:38 EST
Not having python-twisted-core pushed out (at least to devel) will make testing
builds with mock harder, as you'll have to set up a local repo with
python-twisted-core in it and configure mock to look at it.
Comment 22 Thomas Vander Stichele 2006-11-22 20:38:07 EST
Re #21, that's one of the features mock removed when it forked off mach.  With
mach, you can ask it to build a set of spec files and it will figure out
dependencies on its own.

Anyway, still looking for a suggestion, if no suggestion in the next few days I
will go with putting it in there.
Comment 23 Gianluca Sforna 2006-11-23 03:59:37 EST
My 2 cents here: if some breakage should happen because of missing pieces, we
should start building _only_  to -devel, where breakage is at least tolerated
(if not expected ;) ).

When the chain is complete, start builds for FC-6 
Comment 24 Paul Howarth 2006-11-23 04:51:40 EST
There shouldn't be any breakage until the python-twisted metapackage is
released; I really don't see there being an issue with the other packages.
Comment 25 Kevin Fenzi 2006-12-16 14:22:47 EST
The one issue with this package not being built for fc6 at least is that if it
was there the flumotion package could be built, which would fix a E-V-R problem
with that package. (flumotion in fc5 is newer than fc6, so people who upgrade
don't get an updated package). 

In any case I am trying to move forward all the other python-twisted-* reviews. 

Are there any that are not submitted yet? Or can we get this in once the other
pending ones are finished? 
Comment 26 Christopher Stone 2006-12-17 19:19:16 EST
Hi, I need this for poker-network bug #219972
Comment 27 Christopher Stone 2006-12-18 16:32:45 EST
Hi, if this package is dependent on other packages can you please add those to
the depend list.  If not can you please go ahead and push this out for devel and
FC6?  Thanks.
Comment 28 Christopher Stone 2006-12-18 18:49:05 EST
I have placed all the dependencies in bug #171543 so now this one can be closed
and pushed for devel, then when all sub packages are done you can push to fc6
and close 171543.  Does this make sense? I need python-twisted-core and
python-twisted-web as soon as possible for my package review.  Please let me
know if I have missed any dependencies for bug #171543.  Thanks.
Comment 29 Thomas Vander Stichele 2006-12-19 09:32:40 EST
pushed to devel, keeping open for an FC-6 push
Comment 30 Lubomir Kundrak 2008-02-17 11:16:26 EST
Thomas: I'm interested in EL-5 branch. Would you maintain it yourself or you
won't mind if I maintained it?
Comment 31 Lubomir Kundrak 2008-02-20 06:45:44 EST
This is a blocker for my package and the Fedora maintainer seems not to be
interested. cvsextras commits will be open and I'm willing aprove the Fedora
maintainer once he applies for ACLs.

Package Change Request
======================
Package Name: python-twisted-core
New Branches: EL-5
Cvsextras Commits: yes
Comment 32 Kevin Fenzi 2008-02-20 15:16:31 EST
cvs done.

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