Spec Name or Url: http://jdub.homelinux.org/files/gquilt/gquilt.spec SRPM Name or Url: http://jdub.homelinux.org/files/gquilt/gquilt-0.12-1.src.rpm Description: quilt is a tool for managing a series of patches by keeping track of the changes each patch makes. Patches can be applied, un-applied, refreshed, etc. gquilt is a PyGTK GUI wrapper for quilt.
Created attachment 118009 [details] diff between original and patched spec file There are quite a few items that need resolving in this spec before I can review it (unless you want a page full of blockers). If you've got any questions, lemme know. I'll be happy to review a new spec with those changes made.
Patch applied. New SRPM and spec file at the same URL: http://jdub.homelinux.org/files/gquilt/ Some of these things can be fixed upstream too, like the missing COPYING file. I'll be sure to send these changes to the author as well. Thanks for reviewing.
Review: Good: - rpmlint checks returns only E: gquilt only-non-binary-in-usr-lib, which can be ignored, since this is python. - package meets naming/packaging guidelines - license (GPL) ok, license text in %doc, matches source - spec legible, in am.english - sources match upstream - compiles on FC4 (ppc) - no missing BR, no unnecessary BR - no locales, no libs - not relocatable - owns all directories it makes - no duplicate %files - no need for -docs, -devel - code not content - %clean ok - macros are consistent - nothing in %doc affects runtime APPROVED.
Created attachment 118014 [details] Spec patch to fix python compilation and installation Here's some additional problems (patch applied on top of spot's changes): - gquilt installs into /usr/lib. If it contains arch dependent files it should install to %{_libdir} (/usr/lib64 on x86_64). Since it does not install any arch dependent files it should install to %{_datadir}. - On FC4, python .pyo files are not included and %ghosted. I think (no testing) devel will automatically create them and your present %files list will catch them as regular (as opposed to %ghost) files. I'm including something that works for FC4 and earlier. - The generated python byte-compiled files have the wrong path in them (ie: not %{_datadir}/gquilt/gquilt.py) which causes some problems with information shown in exceptions. Here's a patch to the spec for these issues. Two patches used by this will be added next. It's probably worthwhile for you to try to upstream both patches as they will apply to other builders of gquilt as well.
Created attachment 118015 [details] Fix python building to reflect proper locations of byte-compiled files. * Byte compilation that accounts for the proper path to the python files * Install *.py and *.pyo files, not just *.pyc * Change cp's to install and specify mode when installing
Created attachment 118016 [details] Change moduledir from /usr/lib to /usr/share /usr/lib{64,} is for architecture dependent files. This program has all architecture independent files. Install to /usr/share instead.
oops... bugzilla let me slip those in after the review proper. These fixes can go in after the package goes into CVS but they need to be fixed before build otherwise build will fail on x86_64. Second Hmmm... It looks like the python files are used as modules in the %{_datadir}/gquilt hierarchy but the author coded them to act standalone for testing. This is causing rpmlint to throw spurious warnings. (Even /usr/share/gquilt/gquilt.py won't run without the /usr/bin/gquilt wrapper script to set up some ENV variables first.)
(In reply to comment #7) > oops... bugzilla let me slip those in after the review proper. These fixes can > go in after the package goes into CVS but they need to be fixed before build > otherwise build will fail on x86_64. Yep, no problem. And I'll send them upstream to the author too. > > Second Hmmm... It looks like the python files are used as modules in the > %{_datadir}/gquilt hierarchy but the author coded them to act standalone for > testing. This is causing rpmlint to throw spurious warnings. (Even > /usr/share/gquilt/gquilt.py won't run without the /usr/bin/gquilt wrapper script > to set up some ENV variables first.) I'm not a python guru just quite yet... could you elaborate on that a bit more?
The files in /usr/share/gquilt/*.py are imported into the gquilt program to make it run. This is the normal mode of operation. The author of gquilt has also made most of the files executable scripts with small stubs of test code embedded in them. My gquilt-build.patch installs the *.py files as mode 0655... no executable. This seems right as we don't have any use for the tests in the installed set. rpmlint complains about this but I think we should just ignore it. The right thing to do in the python world WRT tests is to move the tests out to separate files. Python has a fine unittest framework to do just that. However, that's a definite job for an upstream maintainer. The /usr/share/gquilt/gquilt.py file is somewhat special as it is the main driver of the gquilt program and there could be some justification to execute it. However, my testing showed the shell script /usr/bin/gquilt sets some environment variables that are necessary otherwise /usr/share...gquilt.py won't run. So there's no need to make that executable either.
(In reply to comment #9) > The files in /usr/share/gquilt/*.py are imported into the gquilt program to make > it run. This is the normal mode of operation. The author of gquilt has also > made most of the files executable scripts with small stubs of test code embedded > in them. My gquilt-build.patch installs the *.py files as mode 0655... no > executable. This seems right as we don't have any use for the tests in the > installed set. rpmlint complains about this but I think we should just ignore it. Ah, yes I noticed this too. I agree that they shouldn't be executable in the scope of Extras. > > The right thing to do in the python world WRT tests is to move the tests out to > separate files. Python has a fine unittest framework to do just that. However, > that's a definite job for an upstream maintainer. I'll be sure to mention it to him. > > The /usr/share/gquilt/gquilt.py file is somewhat special as it is the main > driver of the gquilt program and there could be some justification to execute > it. However, my testing showed the shell script /usr/bin/gquilt sets some > environment variables that are necessary otherwise /usr/share...gquilt.py won't > run. So there's no need to make that executable either. Ok. Thanks for the explanation. It was quite good. You learn something every day :). Your patches have been added to CVS. I'll work with Peter to see if he wants to incorporate them upstream. This explanation should help.
Packages built sucessfully for FC-3, FC-4, and devel