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.

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

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...

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).

Created attachment 139771 [details]
Fixes for issues in comment #1

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.

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?

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.

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

(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.

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 ?

(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.

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?

Thomas, ping!  I'm sure you must be busy but is there a chance that you could
import this soon?

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.

(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.

(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.

ok, that helps.  Did that step.

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

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.

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.

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.

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 

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.

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? 

Hi, I need this for poker-network bug #219972

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.

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.

pushed to devel, keeping open for an FC-6 push

Thomas: I'm interested in EL-5 branch. Would you maintain it yourself or you
won't mind if I maintained it?

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

cvs done.