Spec URL: http://denisarnaud.fedorapeople.org/sim/airsched/airsched-0.1.0-1.spec SRPM URL: http://denisarnaud.fedorapeople.org/sim/airsched/airsched-0.1.0-1.fc15.src.rpm Description: aims at providing a clean API and a simple implementation, as a C++ library, of a Airline Schedule Management System. It is intended to be used in simulated environments only: it is not designed to work in the real-world of Airline IT operations.
Some quick comments: Drop the README file, as it only contains instructions on how to install. The doc package should require the base package. Drop COPYING from the doc sub-package, as the base package provides it. "This package contains the documentation in the HTML format of the %{name} library." -- That's probably not perfect English. Your devel sub-package does not contain static libraries, as the description claims. State BuildRequires per package -- not per sub-package. The two chmods serve no purpose. You can use the name macro on this line: %{_libdir}/lib*.so.* You're packaging manpage 1 twice. I think, you don't have to package the m4 file. "Install the %{name} package if you need a library for simulated Schedule Management C++ library." -- A library for a library?
(In reply to comment #1) > Some quick comments: Thanks for the comments/feedback! > Drop the README file, as it only contains instructions on how to install. I take it as an encouragement to put more project description into the README :) > The doc package should require the base package. AFAIK, that is not a requirement (https://fedoraproject.org/wiki/Packaging/Guidelines#Documentation and https://fedoraproject.org/wiki/Packaging/Guidelines#Requiring_Base_Package). Indeed, it makes sense to install a doc sub-package without having to install the base package, for instance if you just want to read the documentation before hacking with the software. > Drop COPYING from the doc sub-package, as the base package provides it. Well, you are right when the -doc sub-package depends on the base package. In my case, as the -doc sub-package does not require the base package, it is part of the exception and the license file has to be repeated (see https://fedoraproject.org/wiki/Packaging/Guidelines#Duplicate_Files and https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Subpackage_Licensing). > "This package contains the documentation in the HTML format of the %{name} > library." -- That's probably not perfect English. Thanks. I have re-worded it as following: "This package contains HTML pages, as well as a PDF reference manual, for %{name}. All that documentation is generated thanks to Doxygen (http://doxygen.org). The content is the same as what can be browsed online (http://%{name}.org)." > Your devel sub-package does not contain static libraries, as the description > claims. Thanks. I have re-written it: "This package contains the header files, shared libraries and development helper tools for %{name}. If you would like to develop programs using %{name}, you will need to install %{name}-devel." > State BuildRequires per package -- not per sub-package. AFAIK, there is no such requirement from the guidelines (https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRequires). So, it is a question of taste, then (for instance, many legitimate packages are like mine in Fedora). I personally prefer stating the BR in the sub-packages where they are required (e.g., the doxygen BR comes from the documentation package generation). > The two chmods serve no purpose. You are absolutely right. Since I am also part of upstream, it would not have been clever to fix the file permissions only at the time of packaging, would it? I have removed the corresponding two lines. > You can use the name macro on this line: %{_libdir}/lib*.so.* You are right. It is safer to be explicit (and, in the present case, more consistent with the -devel sub-package specification). > You're packaging manpage 1 twice. Unless I have missed something obvious, * one section-1 manual page is for the main (ELF) binary, namely 'airsched'; * while the other section-1 manual page is for the (Shell) configuration helper, namely 'airsched-config'. The latter manual page is in the -devel sub-package because it servers only the needs of developers using AirSched. > I think, you don't have to package the m4 file. You are right: I do not have to. But I do not want to force everybody to use CMake and/or pkgconfig. Typically, developers, building their own software with the GNU Autotools on top of AirSched, will need an airsched.m4 file. Note that mine is probably far from perfect; but I would say "it is better than nothing" for those needing such a thing (in my team, for instance, we needed it for a long time... before switching to CMake). > "Install the %{name} package if you need a library for simulated Schedule > Management C++ library." -- A library for a library? Thanks. I have re-worded it: "Install the %{name} package if you need a library of basic C++ objects for Airline Schedule Management, mainly for simulation purpose." --------------- I shall very shortly publish the new specification file (and source RPM) URLs.
Spec URL: http://denisarnaud.fedorapeople.org/sim/airsched/airsched-0.1.1-1.spec SRPM URL: http://denisarnaud.fedorapeople.org/sim/airsched/airsched-0.1.1-1.fc15.src.rpm
Please always bump the release number when you publish a new version of your files. Ah, yes, these are different man1 pages.
Oh, that was a new version, sorry!
- Why are you using a %{mydocs} macro and not just delete the installdox files in place and own the %{_docdir}/%{name}-%{version}/ folder in %files? - Why the %{?fedora:BuildArch: noarch}?
(In reply to comment #6) > - Why are you using a %{mydocs} macro and not just delete the installdox files > in place and own the %{_docdir}/%{name}-%{version}/ folder in %files? That is a tricky one :) Michael Schwendt, my wonderful sponsor, explained, in my first review request, that issue and the work around I use ever since: https://bugzilla.redhat.com/show_bug.cgi?id=489233#c9 Indeed, if you have carefully look at the output of rpmbuild, you will see that the %doc macro first deletes the $RPM_BUILD_ROOT%{_dordir} directory, and re-creates just aferwards, in order to install into in the base documentation. ---------- Executing(%doc): /bin/sh -e /var/tmp/rpm-tmp.nJHEil + umask 022 + cd /home/build/dev/packages/BUILD + cd airsched-0.1.1 + DOCDIR=/home/build/dev/packages/BUILDROOT/airsched-0.1.1-1.fc15.x86_64/usr/share/doc/airsched-0.1.1 + export DOCDIR + rm -rf /home/build/dev/packages/BUILDROOT/airsched-0.1.1-1.fc15.x86_64/usr/share/doc/airsched-0.1.1 + /bin/mkdir -p /home/build/dev/packages/BUILDROOT/airsched-0.1.1-1.fc15.x86_64/usr/share/doc/airsched-0.1.1 + cp -pr AUTHORS ChangeLog COPYING NEWS README /home/build/dev/packages/BUILDROOT/airsched-0.1.1-1.fc15.x86_64/usr/share/doc/airsched-0.1.1 + exit 0 -------------- > - Why the %{?fedora:BuildArch: noarch}? Well, EPEL <= 5 does not support multi-arch sub-packages. Indeed, EPEL 6 now supports it. So, I should change that macro into something like: %if %{?fedora} || 0%{?rhel} > 5 BuildArch: noarch %endif
> - Why are you using a %{mydocs} macro and not just delete the installdox files > in place and own the %{_docdir}/%{name}-%{version}/ folder in %files? Hmm, I guess, I never had %doc in a doc package. Then you don't run into that problem. Seems to be the only way to solve this problem, then. (In reply to comment #7) > > - Why the %{?fedora:BuildArch: noarch}? > > Well, EPEL <= 5 does not support multi-arch sub-packages. Indeed, EPEL 6 now > supports it. So, I should change that macro into something like: > %if %{?fedora} || 0%{?rhel} > 5 > BuildArch: noarch > %endif (Your snipped would fail on rhel, because of missing "0" before the fedora macro.) I think the cleanest way would be: %if ! (0%{?rhel} > 5) BuildArch: noarch %endif This also is quite close to the python default macro for python3 subpackages: https://fedoraproject.org/wiki/Packaging:Python#Macros
(In reply to comment #8) > This also is quite close to the python default macro for python3 subpackages: > https://fedoraproject.org/wiki/Packaging:Python#Macros The conditions on versions are the same (fedora > 12 and rhel >5). Thanks for having pointed that out. I was trying to find an elegant way to avoid writing explicitly Fedora version, as Fedora <= 13 is no longer supported (EOL). Therefore, "0%{?fedora} > 12" always evaluates to true on Fedora. Hence, "%{?fedora:BuildArch: noarch}" is more elegant. But I do not know how to combine that form of macro (without explicit version) with the other form (i.e., "0%{?rhel} > 5"). If someone has an idea, do not hesitate :) I'll be on vacations for three weeks, and may therefore not be able to work on that packaging effort before some time. Thanks for having given your feedback and hints!
Spec URL: https://raw.github.com/denisarnaud/fedorareviews/trunk/sim/airsched/airsched_732205/airsched.spec SRPM URL: http://sourceforge.net/projects/air-sched/files/0.1/airsched-0.1.2-1.fc15.src.rpm/download ============================================================ There has been an update for the new StdAir release (stdair-0.43.0-1), and feedback from other reviews has been integrated. For instance, the solution to issue above is: (In reply to comment #9) %if 0%{?fedora} || 0%{?rhel} > 5 BuildArch: noarch %endif
Assuming Volker doesn't want to complete this review, as there is no comment in 2 months. (Sorry, if I was disturbing above, didn't want to interrupt in any way.) Review: - name ok - group ok - BR ok (matter of taste, where to put, would prefer in the top) - arch ok (doc is noarch) - %build ok (optflags are used) - %install ok - %check there - libs correctly installed - no static libs - %files (defattr not needed on fedora, but on el5) so ok - $ rpmlint /home/tom/rpmbuild/SRPMS/airsched-0.1.2-1.fc15.src.rpm /home/tom/rpmbuild/RPMS/*/airsched* 5 packages and 0 specfiles checked; 0 errors, 0 warnings. - source match upstream: 13117047d9672cf73b59fa42e0563ce5 airsched-0.1.2.tar.bz2 - doc doesn't R the main package, but has COPYING: ok SHOULD: - Try to convince yourself to add proper license headers ;) ################################################################################ APPROVED
Thanks Tom for the review! New Package SCM Request ======================= Package Name: airsched Short Description: C++ Simulated Airline Schedule Manager Library Owners: denisarnaud Branches: f14 f15 f16 el4 el5 el6 InitialCC: denisarnaud
Git done (by process-git-requests).
airsched-0.1.2-1.fc16 has been submitted as an update for Fedora 16. https://admin.fedoraproject.org/updates/airsched-0.1.2-1.fc16
airsched-0.1.2-1.fc15 has been submitted as an update for Fedora 15. https://admin.fedoraproject.org/updates/airsched-0.1.2-1.fc15
airsched-0.1.2-1.el6 has been submitted as an update for Fedora EPEL 6. https://admin.fedoraproject.org/updates/airsched-0.1.2-1.el6
airsched-0.1.2-1.fc16 has been pushed to the Fedora 16 testing repository.
airsched-0.1.3-2.fc14 has been submitted as an update for Fedora 14. https://admin.fedoraproject.org/updates/airsched-0.1.3-2.fc14
airsched-0.1.3-2.fc14 has been pushed to the Fedora 14 stable repository.
airsched-0.1.2-1.fc15 has been pushed to the Fedora 15 stable repository.
airsched-0.1.2-1.fc16 has been pushed to the Fedora 16 stable repository.
airsched-0.1.2-1.el6 has been pushed to the Fedora EPEL 6 stable repository.