Bug 223588

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.

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>

Ping?

Ping... Thank you so much.    I will fix and bump today!!

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!

> %{_includedir}/rude/config.h

The directory itself is not included!

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:)

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.

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!

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.

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

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?

Yes, this is my first review.  I failed to set the FE-NEEDSPONSOR block, my
apologies. Should I add it now?

Not necessary - I am going to sponsor you.

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

Matt? What is the status of this package?

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

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?

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