Bug 732205 - Review Request: airsched - C++ Simulated Airline Schedule Manager Library
Summary: Review Request: airsched - C++ Simulated Airline Schedule Manager Library
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Thomas Spura
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 702987
Blocks: 760594 972431
TreeView+ depends on / blocked
 
Reported: 2011-08-20 17:38 UTC by Denis Arnaud
Modified: 2013-06-09 11:07 UTC (History)
4 users (show)

Fixed In Version: airsched-0.1.2-1.el6
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-11-14 00:48:59 UTC
Type: ---
Embargoed:
tomspur: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Denis Arnaud 2011-08-20 17:38:08 UTC
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.

Comment 1 Volker Fröhlich 2011-08-21 00:16:18 UTC
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?

Comment 2 Denis Arnaud 2011-08-21 11:16:02 UTC
(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.

Comment 4 Volker Fröhlich 2011-08-21 19:56:33 UTC
Please always bump the release number when you publish a new version of your files.

Ah, yes, these are different man1 pages.

Comment 5 Volker Fröhlich 2011-08-21 19:57:09 UTC
Oh, that was a new version, sorry!

Comment 6 Thomas Spura 2011-08-21 20:51:48 UTC
- 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}?

Comment 7 Denis Arnaud 2011-08-22 20:58:32 UTC
(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

Comment 8 Thomas Spura 2011-08-25 13:41:21 UTC
> - 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

Comment 9 Denis Arnaud 2011-08-25 16:45:27 UTC
(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!

Comment 10 Denis Arnaud 2011-10-26 14:24:13 UTC
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

Comment 11 Thomas Spura 2011-11-03 21:50:09 UTC
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

Comment 12 Denis Arnaud 2011-11-04 12:50:15 UTC
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

Comment 13 Gwyn Ciesla 2011-11-04 13:02:32 UTC
Git done (by process-git-requests).

Comment 14 Fedora Update System 2011-11-04 18:53:29 UTC
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

Comment 15 Fedora Update System 2011-11-04 18:56:34 UTC
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

Comment 16 Fedora Update System 2011-11-04 18:58:44 UTC
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

Comment 17 Fedora Update System 2011-11-04 21:48:04 UTC
airsched-0.1.2-1.fc16 has been pushed to the Fedora 16 testing repository.

Comment 18 Fedora Update System 2011-11-05 00:32:32 UTC
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

Comment 19 Fedora Update System 2011-11-14 00:48:59 UTC
airsched-0.1.3-2.fc14 has been pushed to the Fedora 14 stable repository.

Comment 20 Fedora Update System 2011-11-14 00:49:59 UTC
airsched-0.1.2-1.fc15 has been pushed to the Fedora 15 stable repository.

Comment 21 Fedora Update System 2011-11-14 00:51:00 UTC
airsched-0.1.2-1.fc16 has been pushed to the Fedora 16 stable repository.

Comment 22 Fedora Update System 2011-11-25 02:06:48 UTC
airsched-0.1.2-1.el6 has been pushed to the Fedora EPEL 6 stable repository.


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