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 223588 - Review Request: rudeconfig - C++ library for manipulating config files
Summary: Review Request: rudeconfig - C++ library for manipulating config files
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ralf Corsepius
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2007-01-20 08:17 UTC by Matt Flood
Modified: 2007-11-30 22:11 UTC (History)
0 users

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-02-22 17:08:41 UTC
Type: ---
Embargoed:
wtogami: fedora-cvs+


Attachments (Terms of Use)

Description Matt Flood 2007-01-20 08:17:19 UTC
Spec URL: http://www.rudeserver.com/config/download/rudeconfig.spec
SRPM URL: http://www.rudeserver.com/config/download/rudeconfig-5.0.4-1.src.rpm
Description: 

rudeconfig is a C++ library for reading, writing and 
manipulating configuration/.ini files.

Thanks in advance for the review.

Comment 1 Ralf Corsepius 2007-01-20 09:32:05 UTC
Congrats, a pretty clean package!

Disable building the static library and I'll approve it.



Minor issue: The website doesn't mention version 5.0.4 yet. ATM, the tarball
only seems to be accessible when directly accessing it (I could access and
verifiy it)

A note to upstream (seemingly you): Your configure scripts supports building
bz2-compressed tarballs - It might we worth considering to using the bz2'ed
tarball instead of the gziped versions. Spares a couple of bytes on disks/CDs etc.

Comment 2 Ralf Corsepius 2007-01-20 12:27:30 UTC
MUSTFIX:

- Disable building the static lib (%configure --disable-static)
  and reflect this change where needed.

- Remove commented out duplicate %configure.
  rpm expands comments which causes %configure to be invoked twice.
  If you want to use % in comments you must use %% instead of %.

COSMETICS:

- Minor typo in changelog:
  ...
  Fri Sep 2 2005 .. server.com?
  ...

  This should read ...server.com>



Comment 3 Ralf Corsepius 2007-02-01 07:52:45 UTC
Ping?

Comment 4 Matt Flood 2007-02-01 17:14:33 UTC
Ping... Thank you so much.    I will fix and bump today!!

Comment 5 Matt Flood 2007-02-01 18:38:40 UTC
Once again, thank you for your review.

I have disabled building the static library.  I also removed the 
corresponding %{_libdir}/*.a from the relevant %files section.

I removed the commented out duplicate %configure 
(thank you for the information regarding rpm expansion of comments)

I fixed the typo that was found in the comment section.  I addressed the same 
typo in the the ChangeLog file.

As per your advice, I have switched to using the bz2'ed tarball instead of the 
gzip'ed ones. (thanks for that advice)

I have increased the release number from 1 to 2.  I apologize in advance if my 
assumption to bump that number was wrong.

I have added comments reflecting the major changes for this release in NEWS, 
ChangeLog, and the specfile.

The URLs for the updated files are:

Spec URL: http://www.rudeserver.com/config/download/rudeconfig.spec
SRPM URL: http://www.rudeserver.com/config/download/rudeconfig-5.0.4-2.src.rpm

I am in the process of making version 5.0.4 "visibly" available on the 
website.

Thank you!

Comment 6 Michael Schwendt 2007-02-01 19:08:14 UTC
> %{_includedir}/rude/config.h

The directory itself is not included!

Comment 7 Matt Flood 2007-02-01 19:48:15 UTC
I am sorry, but I do not understand the problem.  
Looking at the following extract of the current spec file:

--------- current extract of spec file-----
%files devel
%defattr(-,root,root,-)
%doc 

%{_includedir}/rude/config.h
%{_libdir}/*.so
%{_mandir}/man3/*
--------------------------------------------

Do you mean that I have ommitted the directory, and should add something like:

%{_includedir}/rude

Or are you saying that I should omit the directory "rude" and just have:

%{_includedir}/config.h

Or is the blank line that I (accidently) introduced below %doc and before
%{_includedir}/config.h a problem (maybe messed up a diff result)?

Or something else that is way over my head - admittedly, my brain is a bit
slodgy today..

Thank you:)

Comment 8 Michael Schwendt 2007-02-01 20:08:54 UTC
The former.

And that's either:

| %files devel
| %defattr(-,root,root,-)
| %{_includedir}/rude/
| %{_libdir}/*.so
| %{_mandir}/man3/*

Or:

| %files devel
| %defattr(-,root,root,-)
| %dir %{_includedir}/rude
| %{_includedir}/rude/config.h
| %{_libdir}/*.so
| %{_mandir}/man3/*

When you list the binary rpm, you want to see a "drwxr-xr-x" entry
for "/usr/include/rude".

The difference between

| %dir %{_includedir}/rude
| %{_includedir}/rude/config.h

and

| %{_includedir}/rude/

is that the latter includes the directory "rude" and the entire
tree below it. On the contrary, with %dir you can include specific
directories, but you need to specify any files in addition to that.


Comment 9 Matt Flood 2007-02-01 23:43:56 UTC
Thank you so much for explaining that to me.  I have used the 

%dir %{_includedir}/rude
%{_includedir}/rude/config.h

The URLs for the updated files are:

Spec URL: 
http://www.rudeserver.com/config/download/rudeconfig.spec

SRPM URL: 
http://www.rudeserver.com/config/download/rudeconfig-5.0.4-3.src.rpm

Thanks again!




Comment 10 Ralf Corsepius 2007-02-02 01:37:20 UTC
Packaging-wise *-3.src.rpm seems fine.

There remains one (upstream) issue:

/usr/include/rude/config.h
uses INCLUDED_CONFIG_H as header guard.

This isn't necessarily a clever choice, because INCLUDED_CONFIG_H is a pretty
generic name which likely to conflict with other package's defines.

I'd recommend to use _RUDE_CONFIG_H or similar.

Comment 11 Matt Flood 2007-02-02 04:19:07 UTC
Thank you.  I have modified the config.h to use the more unique include guard:

INCLUDED_RUDE_CONFIG_H

As a result, I bumped the package version up to 5.0.5 and set the release to 1

http://www.rudeserver.com/config/download/rudeconfig.spec
http://www.rudeserver.com/config/download/rudeconfig-5.0.5-1.src.rpm

Comment 12 Ralf Corsepius 2007-02-02 05:01:48 UTC
APPROVED

(In reply to comment #11)
> Thank you.  I have modified the config.h to use the more unique include guard:
> 
> INCLUDED_RUDE_CONFIG_H

OK, if you prefer it this way :)

BTW: I can't find your name in owners.list - Could it be you need to a sponsor?



Comment 13 Matt Flood 2007-02-02 18:37:31 UTC
Yes, this is my first review.  I failed to set the FE-NEEDSPONSOR block, my
apologies. Should I add it now?

Comment 14 Ralf Corsepius 2007-02-03 15:13:23 UTC
Not necessary - I am going to sponsor you.

Please proceed with step 10 as described on
http://fedoraproject.org/wiki/Extras/Contributors

Comment 15 Ralf Corsepius 2007-02-20 11:29:48 UTC
Matt? What is the status of this package?

The package doesn't seem be imported into FE's cvs, yet.


Comment 16 Matt Flood 2007-02-20 16:33:32 UTC
My apologies, 
  I am currently hung up on the "Identify Yourself as the Owner of the 
Package". It seems that "CVSAdminProcedure" is different now than it was 2 
weeks ago when I went out of town for a conference.  At the time, I had to 
modify some wiki page to get the cvs directories built.  Now I cannot find 
that wiki and this seems to be the first time I have seen the 
CVSAdminProcedure, which I did not do. Nevertheless, the cvs directories were 
created, and I have just imported the package, but I don't seem to have 
requested FC5 and FC6  branches.  Is this something I need to do as a comment 
here?  Or are FC5 and FC6 not applicable for development libraries?

Comment 17 Matt Flood 2007-02-21 17:09:37 UTC
New Package CVS Request
=======================
Package Name: rudeconfig
Short Description: C++ library for manipulating configuration and .ini files
Owners: matt
Branches: FC-6 FC-5
InitialCC: mflood


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