Bug 457212 - Review Request: gadmin-squid - graphical configuration tool for squid
Summary: Review Request: gadmin-squid - graphical configuration tool for squid
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Kevin Fenzi
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-07-30 09:41 UTC by S.A. Hartsuiker
Modified: 2008-09-11 17:18 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-08-12 03:22:48 UTC
Type: ---
Embargoed:
kevin: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description S.A. Hartsuiker 2008-07-30 09:41:38 UTC
Spec URL: http://www.kanarip.com/custom/SPECS/gadmin-squid.spec
SRPM URL: http://www.kanarip.com/custom/f9/SRPMS/gadmin-squid-0.1.0-0.1.fc9.src.rpm
Description: GAdminSquid is a C/GTK+ graphical user interface for the Squid proxy server.

rpmlint -i is quiet

Comment 1 Kevin Fenzi 2008-07-31 06:04:23 UTC
I would be happy to review this... look for a full review probibly tomorrow. 


Comment 2 Kevin Fenzi 2008-08-01 19:31:20 UTC
Sorry for the delay...

OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
See below - License
See below- License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
cdb23e367fc0d2f18c3717aabdc9ef21  gadmin-squid-0.1.0.tar.gz
cdb23e367fc0d2f18c3717aabdc9ef21  gadmin-squid-0.1.0.tar.gz.1
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

OK - Package is a GUI app and has a .desktop file

OK - Package compiles and builds on at least one arch. 
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
OK - final provides and requires are sane.

SHOULD Items:

OK - Should build in mock.
OK - Should have dist tag
OK - Should package latest version

Issues:

1. Not sure the URL is right here. That site mentions that there is a new site
called www.gadmintools.org, which appears to be a dyndns redirect. In any case,
can you find a url that will point a user directly to the gadmin-squid page?

2. The license tag here seems to be 'GPLv3+' to me... ie, GPLv3 or later.

3. Might include the TODO and NEWS files as docs.

4. rpmlint says:

gadmin-squid.src: W: mixed-use-of-spaces-and-tabs (spaces: line 14, tab: line 1)

Can be ignored.

5. Why put the doc files in %{_docdir}/%{name}/ instead of just marking them
%doc and getting them under the standard place:
%{_docdir}/%{name}-%{version}-%{release}/ ?


Comment 3 S.A. Hartsuiker 2008-08-02 23:53:36 UTC
I updated the spec file to reflect the issues raised by Kevin:

SPEC: http://www.kanarip.com/custom/SPECS/gadmin-squid.spec
SRPM: http://www.kanarip.com/custom/el5/SRPMS/gadmin-squid-0.1.0-0.3.el5.src.rpm

point 1: I don't know if the URL I updated to is a good one, but it is better than the previously used one, I'm sure.
point 2: although the license included is GPLv3, the src/* indicates indeed GPLv3+, so changed in the spec.
point 3: files added
point 4: although no problem, changed anyway :)
point 5: missed that one, upstream does it that way, changed in a patch and will contact upstream about it.

Comment 4 Kevin Fenzi 2008-08-03 03:59:49 UTC
Humm... all the issues I saw appear to be solved, but you made an additional change I am unclear on:

Why add:

BuildRequires: autoconf
BuildRequires: automake

and change the configure line to use
%GNUconfigure

?

Comment 5 S.A. Hartsuiker 2008-08-03 11:54:44 UTC
Kevin,

the BuildRequires and %GNUconfigure were neccesary to have it build for epel-5.
The source tarball contains an Makefile.in that borks the build for epel-5.
Including above statements ensures that Makefile.in is regenerated.

Comment 6 Kevin Fenzi 2008-08-04 18:59:19 UTC
Looks like the docs are going to require a bit more tinkering... the latest package doesn't build here. I get: 

error: Installed (but unpackaged) file(s) found:
   /usr/share/doc/gadmin-squid/AUTHORS
   /usr/share/doc/gadmin-squid/COPYING
   /usr/share/doc/gadmin-squid/ChangeLog
   /usr/share/doc/gadmin-squid/README
    Installed (but unpackaged) file(s) found:
   /usr/share/doc/gadmin-squid/AUTHORS
   /usr/share/doc/gadmin-squid/COPYING
   /usr/share/doc/gadmin-squid/ChangeLog
   /usr/share/doc/gadmin-squid/README

If GNUconfigure is needed for EL-5, perhaps only use it in that branch, and/or use a conditional on the %dist tag.

Comment 7 S.A. Hartsuiker 2008-08-04 22:37:45 UTC
I cannot reproduce your results Kevin.
Even on (fresh) bare metal never before used to build this package (or any other) I cannot reproduce this.
What was the command line you used?
As a side note I must say that EPEL-5 is not really a target, more a nice to have at this point.

Comment 8 Kevin Fenzi 2008-08-04 23:04:07 UTC
I was building it here using 'mock'. 
See: http://fedoraproject.org/wiki/PackageMaintainers/MockTricks

for more info. 

I can also submit it as a scratch build in our buildsystem:
http://koji.fedoraproject.org/koji/taskinfo?taskID=758305

which seems to have finished fine. ;( 
I seem to have used the wrong spec or something is going on with my mock machine. ;( 

So, provided you conditionalize the GNUconfigure bits or only check in that version on the EL-5 branch, this package is APPROVED. 

I will go ahead and sponsor you, you can continue the process at: 
http://fedoraproject.org/wiki/PackageMaintainers/Join#Add_Package_to_CVS_and_Set_Owner

Comment 9 S.A. Hartsuiker 2008-08-05 12:29:28 UTC
Kevin, how's this?

new Spec URL: http://www.kanarip.com/custom/SPECS/gadmin-squid.spec
new SRPM URL: http://www.kanarip.com/custom/f9/SRPMS/gadmin-squid-0.1.0-0.4.fc9.src.rpm

Changelog:
- Made autoconf and automake buildrequires conditional and remove
  %GNUconfigure from the .spec
- Added %{__aclocal} and %{__automake} to %prep stage in the .spec, because of outdated files inside the upstream tarball
- replaced another 'bare' make statement with %{__make} in the %install
  stage in the .spec

Comment 10 S.A. Hartsuiker 2008-08-05 12:39:55 UTC
New Package CVS Request
=======================
Package Name: gadmin-squid
Short Description: graphical configuration tool for squid
Owners: baard
Branches: F-8 F-9 
InitialCC: [none]
Cvsextras Commits: yes

Comment 11 Kevin Fenzi 2008-08-05 16:16:22 UTC
Looks ok. Can you ping upstream about including those fixes and/or rebuilding with fixed/newer auto* tools? 

cvs done.

Comment 12 Fedora Update System 2008-08-11 12:47:27 UTC
gadmin-squid-0.1.0-0.4.fc8 has been submitted as an update for Fedora 8

Comment 13 Fedora Update System 2008-08-11 12:49:02 UTC
gadmin-squid-0.1.0-0.4.fc9 has been submitted as an update for Fedora 9

Comment 14 Kevin Fenzi 2008-08-12 03:22:48 UTC
This has been imported and built. Closing now.

Comment 15 Fedora Update System 2008-09-11 16:55:18 UTC
gadmin-squid-0.1.0-0.4.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 16 Fedora Update System 2008-09-11 17:18:38 UTC
gadmin-squid-0.1.0-0.4.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.


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