Bug 457212

Summary: Review Request: gadmin-squid - graphical configuration tool for squid
Product: [Fedora] Fedora Reporter: S.A. Hartsuiker <s.a.hartsuiker>
Component: Package ReviewAssignee: Kevin Fenzi <kevin>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, notting
Target Milestone: ---Flags: kevin: fedora-review+
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: 2008-08-12 03:22:48 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:

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.