Bug 165913 - Review Request: gquilt - a PyGTK GUI wrapper for quilt
Review Request: gquilt - a PyGTK GUI wrapper for quilt
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Tom "spot" Callaway
David Lawrence
http://users.bigpond.net.au/Peter-Wil...
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-08-13 22:15 EDT by Josh Boyer
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-08-24 22:39:28 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
diff between original and patched spec file (2.46 KB, patch)
2005-08-23 12:03 EDT, Tom "spot" Callaway
no flags Details | Diff
Spec patch to fix python compilation and installation (1.57 KB, patch)
2005-08-23 14:26 EDT, Toshio Kuratomi
no flags Details | Diff
Fix python building to reflect proper locations of byte-compiled files. (2.71 KB, patch)
2005-08-23 14:32 EDT, Toshio Kuratomi
no flags Details | Diff
Change moduledir from /usr/lib to /usr/share (957 bytes, patch)
2005-08-23 14:34 EDT, Toshio Kuratomi
no flags Details | Diff

  None (edit)
Description Josh Boyer 2005-08-13 22:15:26 EDT
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.
Comment 1 Tom "spot" Callaway 2005-08-23 12:03:02 EDT
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.
Comment 2 Josh Boyer 2005-08-23 14:00:08 EDT
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.
Comment 3 Tom "spot" Callaway 2005-08-23 14:12:25 EDT
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.
Comment 4 Toshio Kuratomi 2005-08-23 14:26:27 EDT
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.
Comment 5 Toshio Kuratomi 2005-08-23 14:32:55 EDT
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
Comment 6 Toshio Kuratomi 2005-08-23 14:34:59 EDT
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.
Comment 7 Toshio Kuratomi 2005-08-23 14:52:43 EDT
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.)
Comment 8 Josh Boyer 2005-08-23 16:18:29 EDT
(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?

Comment 9 Toshio Kuratomi 2005-08-23 17:45:50 EDT
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.
Comment 10 Josh Boyer 2005-08-23 21:03:49 EDT
(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.

Comment 11 Josh Boyer 2005-08-24 22:39:28 EDT
Packages built sucessfully for FC-3, FC-4, and devel

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