This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 223588 - Review Request: rudeconfig - C++ library for manipulating config files
Review Request: rudeconfig - C++ library for manipulating config files
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ralf Corsepius
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2007-01-20 03:17 EST by Matt Flood
Modified: 2007-11-30 17:11 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-02-22 12:08:41 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Matt Flood 2007-01-20 03:17:19 EST
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 04:32:05 EST
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 07:27:30 EST
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 02:52:45 EST
Ping?
Comment 4 Matt Flood 2007-02-01 12:14:33 EST
Ping... Thank you so much.    I will fix and bump today!!
Comment 5 Matt Flood 2007-02-01 13:38:40 EST
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 14:08:14 EST
> %{_includedir}/rude/config.h

The directory itself is not included!
Comment 7 Matt Flood 2007-02-01 14:48:15 EST
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 15:08:54 EST
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 18:43:56 EST
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-01 20:37:20 EST
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-01 23:19:07 EST
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 00:01:48 EST
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 13:37:31 EST
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 10:13:23 EST
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 06:29:48 EST
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 11:33:32 EST
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 12:09:37 EST
New Package CVS Request
=======================
Package Name: rudeconfig
Short Description: C++ library for manipulating configuration and .ini files
Owners: matt@rudeserver.com
Branches: FC-6 FC-5
InitialCC: mflood@fuelcellstore.com

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