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 188351 - Review Request: gphpedit - GNOME2 PHP editor
Summary: Review Request: gphpedit - GNOME2 PHP editor
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Christoph Wickert
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-04-08 09:20 UTC by Tim Jackson
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-04-17 20:37:28 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Tim Jackson 2006-04-08 09:20:03 UTC
Spec Name or Url: http://www.timj.co.uk/linux/specs/gphpedit.spec
SRPM Name or Url: http://www.timj.co.uk/linux/srpms/gphpedit-0.9.80-1.src.rpm
Description: 
gPHPEdit is a GNOME2 editor dedicated to editing PHP files and other 
supporting files like HTML/CSS.

Comment 1 Christoph Wickert 2006-04-13 22:45:35 UTC
Hi Tim, just a few comments:

- Source0 needs an absolute URL like http://...
- The %files section looks broken, the package doesn't own %{_datadir}/gphpedit/. 
- You are replacing a hardcoded path with another hardcoded one in the specfile.
Better use
sed -i s_/usr/local/share/pixmaps_%{_datadir}/pixmaps_ src/main.h
- Please add the categories "GNOME" and "Application" to gphpedit.desktop

The rest looks good to me. I'm going to do a review ASAP.

Comment 2 Tim Jackson 2006-04-13 23:17:04 UTC
Thanks for the comments Christoph! All fixed, with the exception of Source0: the
upstream site doesn't have a proper URL to download from (it is obscured by a
download hit-tracker). I've already requested that the author adds a proper URL
(e.g. http://www.gphpedit.org/downloads/gphpedit-%{version}.tar.gz). 

New SRPM:
http://www.timj.co.uk/linux/srpms/gphpedit-0.9.80-2.src.rpm
Spec in same place as before.

Comment 3 Christoph Wickert 2006-04-15 00:59:11 UTC
REVIEW for
f8cf9167b19c07f202d2d5e31431648a  gphpedit-0.9.80-2.src.rpm

Good:
- rpmlint clean except for one zero-length error (see below)
- package and specfile naming according to Package Naming Guidelines
- package meets Packaging Guidlines
- license ok (GPL)
- license field in specfile machtes actual license
- license included in source and correctly installed in %doc
- specfile written in American English
- specfile legible. Actually that's what specs look like! :-) Clean and well
commented.
- source matches upstream
- package builds into binaries in Fedora Core 5 i386
- no locales to worry about
- no shared libs
- relocatable
- package owns all directories it creates
- package doesn't own files or dirs already owned by other packages
- no duplicates in %files listing
- permissions of files ok, %defattr correct
- %clean section present and correct
- macro usage consistent
- code, not content
- no large docs
- docs don't affect runtime
- no headers, pkgconfigs or static libs to worry about
- no libtool archives
- desktop file included and correctly installed

- package builds in mock for Core 5 i386
- program works fine, package has been testet for more than a week without problems
- changelog information correct and detailed 


APPROVED


You can now import the srpm into CVS, but there are four things you need to fix
before building:

- remove gtk2-devel from BuildRequires. It's a duplicate as it's already
required both by gtkhtml2-devel and libgnomeui-devel. The rest of the Requires
and BuildRequires are correct, none of the exceptions listed in the wiki.

- remove empty NEWS from %doc to fix this rpmlint error.
> rpmlint gphpedit-0.9.80-2.fc5.i386.rpm
> E: gphpedit zero-length /usr/share/doc/gphpedit-0.9.80/NEWS

- remove generic INSTALL from %doc (not needed)

- please add the URL for Source0 when you receive feedback from upstream

Comment 4 Tim Jackson 2006-04-15 02:04:23 UTC
Thanks very much for the review Christoph.
Imported into CVS; the first build failed on x86_64
(http://buildsys.fedoraproject.org/build-status/job.psp?uid=7737) so I will look
into that. At a first glance it looks like something fishy upstream with
precompiled libraries bundled in the source tarball, so the spec may need
revisiting to rebuild those properly and appropriately for the arch.

Comment 5 Warren Togami 2006-04-17 15:32:07 UTC
Precomplied binaries are unacceptable.  It must be built from scratch, and maybe
in its own package that this depends on.

Comment 6 Tim Jackson 2006-04-17 20:15:26 UTC
Yes, I know. Looking at it, it appears that the presence of the pre-existing
binaries may actually be accidental (perhaps an error during the release
process?); I will discuss with the author. A "make clean; make" recompiles the
objects in question cleanly, at least on x86; I will submit a new build shortly.
The presence of the library (gtkscintilla, "moleskine" version) bundled in the
package is intentional; the library is a modified version (compatible licensing
- GPL) and is effectively unmaintained upstream so the gPHPEdit author hasn't
got much choice but to bundle it. There were apparently extensive discussions
during the Debian packaging process for the same reason (I understand they hate
multiple copies of libraries with a vengeance) and it was agreed there that
bundling is appropriate in this case. (Note I am treating "bundling of
libraries" and "distribution of precompiled binaries" as totally separate issues
here)

Comment 7 Tim Jackson 2006-04-17 20:37:28 UTC
Looking healthier now; the current version now builds on all archs.

Comment 8 Tim Jackson 2006-04-18 21:17:00 UTC
Upstream author has confirmed that the inclusion of precompiled binaries was an
error in the release process; next upstream version won't have them and so the
"make clean" hack can be dropped.


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