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
Summary: | Review Request: rudeconfig - C++ library for manipulating config files | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Matt Flood <matt> |
Component: | Package Review | Assignee: | Ralf Corsepius <rc040203> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | Flags: | wtogami:
fedora-cvs+
|
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2007-02-22 17:08:41 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Matt Flood
2007-01-20 08:17:19 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. 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 |