Bug 479951 - Review Request: iniparser - a library for parsing ini-style files
Review Request: iniparser - a library for parsing ini-style files
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-14 04:24 EST by Alex Hudson (Fedora Address)
Modified: 2013-06-23 17:46 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-05-24 07:12:48 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Alex Hudson (Fedora Address) 2009-01-14 04:24:41 EST
Spec URL: http://www.alexhudson.com/fedora/iniparser/iniparser.spec
SRPM URL: http://www.alexhudson.com/fedora/iniparser/iniparser-3.0b-1.fc10.src.rpm
Description: 
IniParser is a simple ANSI-C library used by other applications to parse "ini-style" files as used mainly on Windows.

There is a small problem with this library; it doesn't come with a "full" build system - it has a Makefile which compiles the library, but doesn't even have "make install", thus the manual fiddling around in the spec file.

I suspect this makes the -debuginfo package less useful - it doesn't come with copies of the source, for example. 

Another issue is that the library is simply called libiniparser.so.0 - there is no minor version. I imagine this would make it difficult to correctly work out dependencies (for example, if new API was added to the library later and an application wished to declare that dependency) - however, there is also a compatibility issue there if it's changed just for Fedora?

I'm contemplating writing a patch which would effectively drop in a more standard autotools build, but would appreciate any easier ideas.

Also, this is only the second package I've submitted and I'm not a package maintainer (yet, hopefully ;)
Comment 1 Jochen Schmitt 2009-01-14 11:57:26 EST
Good:
+ Package name fits with nameing guidelines.
+ Source file could downloaded with spectool
+ Tar ball in package fits with upstreams
(md5sum: 89b7d97b9fb24ce4c31743cc4d13ce44)
+ Package contains valid Group
+ Package contains valid license tag
+ License from the license tag is a valid OSS license
+ Consistently usage of rpm macros
+ Package contains varbatin copy of the license text
+ Rpmlint is quite on source rpm
+ Rpmlint is quite on binary rpm
+ Rpmlint is quite on intalled rpm
+ Rpmlint is quite on debuginfo package, Debuginfo package
contains valid source files.
+ Package contains scriptlets for ldconfig
+ Package contains a devel subpackage
+ devel subpackage contains Requires on main package
+ Files contains no files belongs to other packages
+ Files have proper file perms
+ %doc stanza is small, so we need no separate doc subpackage
+ Package builds fine on koji, if you are add '-fPIC'
+ Package contains proper %changelog stanza
+ Local install of the package works fine
+ Local uninstall of the package works fine

Bad:
- Local build failed, Please add '-fPIC' compilerflag if you want to create a dynamic library
- Please use install for installing the files of your package 
- Please notify upstream, that each source file should have a copyright notice 
- Please remove %check stanza, if you don't have the testsuite which should be executing during the build
Comment 2 Alex Hudson (Fedora Address) 2009-01-14 19:16:52 EST
Jochen,

Thank you very much for taking the time out to review my package, much appreciated. I've updated here (SRPM has changed):

Spec URL: http://www.alexhudson.com/fedora/iniparser/iniparser.spec
SRPM URL:
http://www.alexhudson.com/fedora/iniparser/iniparser-3.0b-2.fc10.src.rpm

I've addressed the three points you've raised about the .spec, and will engage upstream on the copyright issue.

I did a scratch build on Koji (task id #1054148), and can confirm it builds - sorry, I should have done that originally.
Comment 3 Jochen Schmitt 2009-01-18 11:55:14 EST
Good:
+ Local build works fine.
+ Files are install with the install programm.
+ Empty %check stanza is removed.

Bad:
- Package doesn't contains verbatin copy of the license text, but upstream tar ball contains a LICENSE file.

If you are add the LICENSE file into the %doc stanza you package is APPROVED.
Comment 4 Mamoru TASAKA 2009-01-18 12:48:43 EST
Hello, Jochen. Are you a sponsor? (sorry if you are so)

Any NEEDSPONSOR blocking review tickets needs the approval
by sponsor members.
Comment 5 Alex Hudson (Fedora Address) 2009-01-19 05:29:16 EST
Many thanks again, Jochen - I've added the LICENSE and bumped the rpm.

Spec URL: http://www.alexhudson.com/fedora/iniparser/iniparser.spec
SRPM URL: http://www.alexhudson.com/fedora/iniparser/iniparser-3.0b-3.fc10.src.rpm
Comment 6 Jochen Schmitt 2009-01-19 11:18:09 EST
Sorry, I'm not a sponsor.
Comment 7 Kevin Fenzi 2009-01-25 15:26:55 EST
Hey Alex. I would be happy to look at this and bug 479835. 

Look for more info later today.
Comment 8 Kevin Fenzi 2009-01-25 16:01:58 EST
On quick comment here: 

Is this a pre-release/post-release or snapshot package? 
If so, see: http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Non-Numeric_Version_in_Release

Ie, is it a beta of an upcoming 3.0 release? If so the version should probibly be: 3.0-0.1.b
Comment 9 Alex Hudson (Fedora Address) 2009-01-26 11:32:58 EST
Hi Kevin,

Thanks for taking a look at this!

You're right about the version number - for some reason, I had assumed that this was a subsequent release to 3.0, but it appears not. I've updated the spec. to take into account the pre-release numbering:

http://www.alexhudson.com/fedora/iniparser/iniparser.spec

This is actually rpm-older than the previous version, but hopefully that's not really a problem at this point.

Also, while this is "pre-release", iniparser seems to be developed quite slowly and conservatively, and I would expect most software that uses it would expect this more up-to-date version.
Comment 10 Kevin Fenzi 2009-01-28 01:13:30 EST
I don't see any further issues here, so this package is APPROVED.

removing NEEDSPONSOR as I am sponsoring Alex. 

Thanks for reviewing this Jochen!
Comment 11 Kevin Fenzi 2009-05-11 02:40:10 EDT
Alex: Care to request cvs here?
Comment 12 Jochen Schmitt 2009-05-12 12:32:59 EDT
Yes I missing the CVS-Admin request to, so what is the state of this ticket?
Comment 13 Alex Hudson (Fedora Address) 2009-05-15 02:39:47 EDT
New Package CVS Request
=======================
Package Name: iniparser
Short Description: a library for parsing ini-style files 
Owners: alexh
Branches: F-10 F-11
InitialCC: 

Apologies for dropping the ball on this one; my dev. machine was out for a while and I'd forgotten it was outstanding - thanks for reminding me!
Comment 14 Kevin Fenzi 2009-05-15 19:44:21 EDT
cvs done.
Comment 15 Alex Hudson (Fedora Address) 2009-05-24 07:12:48 EDT
Imported and built, thanks.
Comment 16 Joseph Marrero 2013-06-23 17:45:22 EDT
Package Change Request
======================
Package Name: iniparser
New Branches: el6
Owners: jcapik jmarrero

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