Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 208169
Summary: | Review Request: python-twisted-core - An asynchronous networking framework written in Python | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Thomas Vander Stichele <thomas> | ||||
Component: | Package Review | Assignee: | Paul Howarth <paul> | ||||
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | medium | ||||||
Version: | rawhide | CC: | jeff, kevin, lkundrak | ||||
Target Milestone: | --- | Flags: | kevin:
fedora-cvs+
|
||||
Target Release: | --- | ||||||
Hardware: | All | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2006-12-27 14:53:00 UTC | Type: | --- | ||||
Regression: | --- | Mount Type: | --- | ||||
Documentation: | --- | CRM: | |||||
Verified Versions: | Category: | --- | |||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
Cloudforms Team: | --- | Target Upstream Version: | |||||
Embargoed: | |||||||
Bug Depends On: | 207265 | ||||||
Bug Blocks: | 163779, 171543, 219972, 221310 | ||||||
Attachments: |
|
Description
Thomas Vander Stichele
2006-09-26 18:52:22 UTC
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. |