Bug 212894 (libopm) - Review Request: libopm - Blitzed open proxy monitor library
Summary: Review Request: libopm - Blitzed open proxy monitor library
Keywords:
Status: CLOSED NEXTRELEASE
Alias: libopm
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Christopher Stone
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-10-30 00:45 UTC by Robert Scheck
Modified: 2007-11-30 22:11 UTC (History)
0 users

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-11-08 21:54:52 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Robert Scheck 2006-10-30 00:45:08 UTC
Spec URL: http://labs.linuxnetz.de/bugzilla/libopm.spec
SRPM URL: http://labs.linuxnetz.de/bugzilla/libopm-20050731-2.src.rpm
Description: An open proxy detection library, developed by the blitzed IRC 
network team. Its original use was to detect open proxies running on clients 
connecting to various IRC servers, but it has evolved to become a generic
open proxy detection library.

libopm has no version number, because it never got released and there isn't any 
plan to do so, when I talked with libopm upstream earlier this year. And as you 
can see from the changes in the libopm repository, there are only very rarely 
minor changes, so using the date as version number should fit (like practised at 
bash-completion, dumpasn1, geda, gnubg, mbuffer and further packages in Fedora 
Extras).

Comment 1 Christopher Stone 2006-10-31 03:02:12 UTC
Some initial comments before I can do a formal review:

- Source0 should list the full download URL and use %{name}
- make should use $RPM_OPT_FLAGS
- use %defattr(-,root,root,-)
- You need to mention in a comment how to download the source if there is not a
direct URL
- I think the group should be Development/Libraries for the main package as
well, although I cannot find anything which specifies how the groups are defined
- You should use --disable-static on the %configure
- I think the version number should be done like so:

Version: 0.0.0
Release: 0.2.20050731cvs%{dist}

See
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-d97a3f40b6dd9d2288206ac9bd8f1bf9b791b22a


Comment 2 Robert Scheck 2006-10-31 13:10:12 UTC
> - Source0 should list the full download URL and use %{name}
> - You need to mention in a comment how to download the source if there is
    not a direct URL

Fixed; you're right of course.

> - make should use $RPM_OPT_FLAGS

Hum? gcc -DHAVE_CONFIG_H -I. -I. -I. -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 
-fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -
mtune=generic -fasynchronous-unwind-tables -c config.c -MT config.lo -MD -MP -MF
 .deps/config.TPlo  -fPIC -DPIC -o .libs/config.lo -- IMHO $RPM_OPT_FLAGS seems 
to be used. What should be wrong here?

> - use %defattr(-,root,root,-)

I'm using %defattr(-,root,root) which fits and does the same. I wasn't able to 
find sth else e.g. at http://fedoraproject.org/wiki/Packaging/ReviewGuidelines 
(which says, that %defattr(...) must be used).

> - I think the group should be Development/Libraries for the main package as
>   well, although I cannot find anything which specifies how the groups are 
>   defined

Why? libtiff, libjpeg, libidn, libselinux, libesmtp, librsync are using the same 
like mine. Why should System Environment/Libraries be wrong? It decribes simply 
perfect the main package of libopm - IMHO.

> - You should use --disable-static on the %configure

Hum? MUST: Header files or static libraries must be in a -devel package. This is 
what I'm following (http://fedoraproject.org/wiki/Packaging/ReviewGuidelines).

> - I think the version number should be done like so:

Looks whether you're right. But I found AC_INIT() in configure.in, which tells 
version 0.1, So it would be the following, right? IMO it must be a post release 
of 0.1, because 0.1 was always set when browsing through viewcvs of Blitzed.

Version: 0.1
Release: 2.20050731cvs%{dist}

I'll push a new build when all things are clarified.

Comment 3 Christopher Stone 2006-11-02 17:26:12 UTC
-For the RPM_OPT_FLAGS, I did not check the actual flags being used.  I simply
noted it because I did not see it explicitly mentioned in the spec file.  I will
take a closer look at this issue when I do the formal review.  You can leave it
out for now if you think it's not needed.

- I suggested you use %defattr(-,root,root,-) because all of the examples given
use this.  It probably doesn't matter much and I wont block the review because
of it, but I don't see any harm in adding it either.

See: http://fedoraproject.org/wiki/Packaging/Guidelines?highlight=%28defattr%29

- The group doesn't matter to me, I can't find anything anywhere defining the
groups so you can leave it as is if you like.

- I suggested you use --disable-static because of this:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-2302ec1e1f44202c9cc4bcce24cb711266557ad7

Which states that static libraries should be disable whenever possible.  So
unless you got a *really* good reason to keep the static library, then I will
allow you to keep it in the package, but you must add a comment in the spec file
explaining the *really* good reason for you to keep it.  Otherwise this is a
blocker and the static library must be removed before I can approve it.

- For the version number, I suggested 0.0.0 because this was the version number
use in the .so filename.  However, if you want to use 0.1, that is fine too. :)


Comment 4 Robert Scheck 2006-11-02 19:25:06 UTC
Okay, hopefully fixed everything; new:

Spec URL: http://labs.linuxnetz.de/bugzilla/libopm.spec
SRPM URL: http://labs.linuxnetz.de/bugzilla/libopm-0.1-3.20050731cvs.src.rpm

Comment 5 Christopher Stone 2006-11-05 21:50:12 UTC
======== REVIEW CHECKLIST ========
- rpmlint output clean
- package named according to package naming guidelines
- spec filename matches package name
- package meets packaging guidelines
- package licensed with opensource compatible license
- license in spec matches actual license
- license in %doc
- spec file written in American English
- spec file is legible
X SOURCE FILES DO NOT MATCH
- package successfully compiles and builds on FC5 x86_64
- no build dependencies required
- no locales required
- %post and %postun properly run ldconfig
- package is not relocatable
- package owns all directories it creates
- no duplicate files in %files
- permissions are set properly
- spec file contains proper %clean section
- macro usage is consistent
- package contains code
- no large documentation files
- files in %doc do not affect runtime
- header files are in devel subpackage
- no pkgconfig used
- .so file in devel
- devel package requires base package
- package does not contain .la file
- not a GUI app, no .desktop file needed
- package does not own files or directories owned by other pacakges

======== MUST FIX ========
X SOURCES DO NOT MATCH
  - 5537327b4e76bd2c5074996b16cad361  ../SOURCES/libopm-0.1.tar.gz
  - c3da807c0d90cc89c301d83ac5669238  cvs/libopm-0.1.tar.gz
  - Please provide specific tar instruction used, I don't want to check the
md5sum of each source file individually.  I believe the difference is caused by
the order of the files in the tarball.  The command I used to tar the source
files was: tar czf libopm-0.1.tar.gz libopm-0.1/
  - In addition, your cvs co command should include a specific date so that if
something is added to the cvs in the future, someone can still pull the exact
sources used in this package.


Comment 6 Robert Scheck 2006-11-05 22:32:28 UTC
Adding date to the checkout command should be no problem (try `cvs -z3 -d:
pserver:anon.org:/ co -D "20050731 23:59" libopm`), but how to 
change the ordering of the files in the tarball I absolutely don't know. All
I did was a "tar cvfz libopm-0.1.tar.gz libopm-0.1" as non-root user. But is 
this really important? 

[22:56:14] < mitr> rsc: Perhaps (tar czf foo.tar.gz $(find libopm-0.1 | LC_ALL=C 
sort)) will make the file order consistent - but I don't know about other 
attributes of the tarball, e. g. owner&group.
[22:56:30] < rsc> hm.
[22:57:21] < mitr> rsc: IMHO it should be enough to compare a checkout and the 
tarball by preparing them and using (diff -urN)
[22:57:23] < ensc> what's the sense of the md5sum check?
[22:57:59] < ensc> packager can rpm-import everything what he wants

Finally the suggested command adds every file twice here...

Comment 7 Christopher Stone 2006-11-06 05:09:14 UTC
$ diff -urN libopm-0.1 ../rpmbuild/SOURCES/libopm-0.1
Output clean.

Please add -D "20050731 23:59" to the comments after checking into CVS.

APPROVED.

Comment 8 Robert Scheck 2006-11-08 21:54:52 UTC
21226 (libopm): Build on target fedora-development-extras succeeded.
21227 (libopm): Build on target fedora-6-extras succeeded.
21228 (libopm): Build on target fedora-5-extras succeeded.


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