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.
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