[Bug 732205] New: Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

Summary: Review Request: airsched - C++ Simulated Airline Schedule Manager Library

https://bugzilla.redhat.com/show_bug.cgi?id=732205

Summary: Review Request: airsched - C++ Simulated Airline
                    Schedule Manager Library
           Product: Fedora
           Version: rawhide
          Platform: All
        OS/Version: Linux
            Status: NEW
          Severity: medium
          Priority: medium
         Component: Package Review
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]
         QAContact: [e-mail]
                CC: [e-mail],
                    [e-mail]
    Classification: Fedora
      Story Points: ---
              Type: ---

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

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

Volker Fröhlich  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #1 from Volker Fröhlich  2011-08-20 20:16:18 EDT ---
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?

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

--- Comment #2 from Denis Arnaud  2011-08-21 07:16:02 EDT ---
(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_Lice...).
> "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://%%7Bname%7D.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.

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

--- Comment #3 from Denis Arnaud  2011-08-21 10:15:52 EDT ---
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.r...

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

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

Ah, yes, these are different man1 pages.

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

--- Comment #5 from Volker Fröhlich  2011-08-21 15:57:09 EDT ---
Oh, that was a new version, sorry!

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

Thomas Spura  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #6 from Thomas Spura  2011-08-21 16:51:48 EDT ---
- 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}?

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

--- Comment #7 from Denis Arnaud  2011-08-22 16:58:32 EDT ---
(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

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

--- Comment #8 from Thomas Spura  2011-08-25 09:41:21 EDT ---
> 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

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

--- Comment #9 from Denis Arnaud  2011-08-25 12:45:27 EDT ---
(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!

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

Denis Arnaud  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |702987

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

--- Comment #10 from Denis Arnaud  2011-10-26 10:24:13 EDT ---
Spec URL:
https://raw.github.com/denisarnaud/fedorareviews/trunk/sim/airsched/airsched...
SRPM URL:
http://sourceforge.net/projects/air-sched/files/0.1/airsched-0.1.2-1.fc15.sr...
============================================================

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

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

Thomas Spura  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
               Flag|                            |fedora-review+

--- Comment #11 from Thomas Spura  2011-11-03 17:50:09 EDT ---
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

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

Thomas Spura  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|[e-mail]    |[e-mail]

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

Denis Arnaud  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-cvs?

--- Comment #12 from Denis Arnaud  2011-11-04 08:50:15 EDT ---
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

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

--- Comment #13 from Jon Ciesla  2011-11-04 09:02:32 EDT ---
Git done (by process-git-requests).

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

Fedora Update System  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |MODIFIED

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

--- Comment #14 from Fedora Update System  2011-11-04 14:53:29 EDT ---
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

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

--- Comment #15 from Fedora Update System  2011-11-04 14:56:34 EDT ---
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

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

--- Comment #16 from Fedora Update System  2011-11-04 14:58:44 EDT ---
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

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

Fedora Update System  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|MODIFIED                    |ON_QA

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

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

--- Comment #18 from Fedora Update System  2011-11-04 20:32:32 EDT ---
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

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

Fedora Update System  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ON_QA                       |CLOSED
   Fixed In Version|                            |airsched-0.1.3-2.fc14
         Resolution|                            |ERRATA
        Last Closed|                            |2011-11-13 19:48:59

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

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

Fedora Update System  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
   Fixed In Version|airsched-0.1.3-2.fc14       |airsched-0.1.2-1.fc15

--- Comment #20 from Fedora Update System  2011-11-13 19:49:59 EST ---
airsched-0.1.2-1.fc15 has been pushed to the Fedora 15 stable repository.

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

Fedora Update System  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
   Fixed In Version|airsched-0.1.2-1.fc15       |airsched-0.1.2-1.fc16

--- Comment #21 from Fedora Update System  2011-11-13 19:51:00 EST ---
airsched-0.1.2-1.fc16 has been pushed to the Fedora 16 stable repository.

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

Fedora Update System  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
   Fixed In Version|airsched-0.1.2-1.fc16       |airsched-0.1.2-1.el6

--- Comment #22 from Fedora Update System  2011-11-24 21:06:48 EST ---
airsched-0.1.2-1.el6 has been pushed to the Fedora EPEL 6 stable repository.

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=732205

Denis Arnaud  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |760594

[Bug 732205] Review Request: airsched - C++ Simulated Airline Schedule Manager Library

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=732205

Denis Arnaud  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |972431