Bug 702987 - Review Request: stdair - C++ Standard Airline IT Library
Summary: Review Request: stdair - C++ Standard Airline IT Library
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Martin Gieseking
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 614036 (view as bug list)
Depends On:
Blocks: 728649 728815 732146 732205 732218 750099 760594 781775
TreeView+ depends on / blocked
 
Reported: 2011-05-08 21:12 UTC by Denis Arnaud
Modified: 2012-01-15 03:09 UTC (History)
5 users (show)

Fixed In Version: rmol-0.25.2-1.el6
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-08-11 23:28:08 UTC
Type: ---
Embargoed:
martin.gieseking: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Denis Arnaud 2011-05-08 21:12:03 UTC
Spec URL: http://denisarnaud.fedorapeople.org/stdair/stdair-0.30.0-1.spec
SRPM URL:
https://downloads.sourceforge.net/project/stdair/fedora_14/stdair-0.30.0-1.fc14.src.rpm
===============================================
Description:
StdAir aims at providing a clean API, and the corresponding C++
implementation, for the basis of Airline IT Business Object Model (BOM),
that is, to be used by several other Open Source projects, such as RMOL, 
Air-Sched, Travel-CCM, OpenTREP, etc.

Comment 1 Denis Arnaud 2011-05-08 21:13:54 UTC
*** Bug 614036 has been marked as a duplicate of this bug. ***

Comment 2 Denis Arnaud 2011-05-08 21:24:15 UTC
StdAir does no longer depends on ExtraCC (see bug #616881); so, the second issue in https://bugzilla.redhat.com/show_bug.cgi?id=614036#c4 does no longer apply. And, as all the other issues had already been addressed, that package should now be correct. Moreover, of course, rpmlint does not report any error/warning.

Comment 3 Denis Arnaud 2011-05-22 23:12:56 UTC
Spec URL: http://denisarnaud.fedorapeople.org/stdair/stdair-0.32.0-1.spec
SRPM URL:
https://downloads.sourceforge.net/project/stdair/fedora_14/stdair-0.32.0-1.fc14.src.rpm
===============================================

Comment 4 Martin Gieseking 2011-07-30 18:43:08 UTC
Some quick comments:
- Would you like to update the package to the latests release?

- As you're one of the upstream developers, please add the LGPL copyright 
  header to all source files as requested by the (L)GPL (see COPYING).

- The package currently doesn't build in mock because of missing BR
  readline-devel

- There's no need to remove INSTALL in %prep.

- Also, it's not necessary to wrap the BuildRoot and BuildArch fields with
  conditionals. Just use plain BuildRoot/BuildArch fields, even if an explicit 
  BuildRoot tag is optional in Fedora and EPEL > 5.

- After the update of the packaging guidelines, the devel package must now 
  require the base package this way:
  Requires: %{name}%{?_isa} = %{version}-%{release}

- The following dirs are unowned:
  /usr/share/stdair/samples
  /usr/share/stdair/samples/rds01

Comment 5 Denis Arnaud 2011-07-30 19:23:16 UTC
Many thanks for your early feedback.

Yes, I will shortly update the package with the latest release (currently version 0.36.0, not yet on SourceForge but the release branch is already existing in Git: https://github.com/denisarnaud/stdair/tree/0.36.0). As a matter of fact, I have just finished the migration to the CMake build system (from the GNU Autotools).

I will shortly work on the other issues. As for the INSTALL file, the guidelines require to remove it: https://fedoraproject.org/wiki/Packaging/Guidelines#Documentation

Again, many thanks for your first review elements, as that package is important for my team. It constitutes the ground work for twelve other components, which all put together offer a nice revenue management simulation engine (http://sourceforge.net/projects/dsim/).

Comment 6 Martin Gieseking 2011-07-30 19:50:47 UTC
(In reply to comment #5)
> As for the INSTALL file, the
> guidelines require to remove it:
> https://fedoraproject.org/wiki/Packaging/Guidelines#Documentation

To be nitpicking: The guidelines require not to package INSTALL files but you don't need to remove it explicitly. If you don't add it as %doc, everything should be fine. However, it doesn't hurt to remove it either. It's just redundant. :)

> Again, many thanks for your first review elements, as that package is important
> for my team. It constitutes the ground work for twelve other components, which
> all put together offer a nice revenue management simulation engine
> (http://sourceforge.net/projects/dsim/).

OK, thanks for the information, and you're welcome. I'll do the formal review if you're recent version is packaged.

Comment 7 Denis Arnaud 2011-07-30 20:49:53 UTC
Spec URL: http://denisarnaud.fedorapeople.org/stdair/stdair-0.36.0-1.spec
SRPM URL:
https://downloads.sourceforge.net/project/stdair/fedora_15/stdair-0.36.0-1.fc15.src.rpm
===============================================

Comment 8 Denis Arnaud 2011-07-30 22:41:45 UTC
I've still to add the license information everywhere...

Comment 9 Denis Arnaud 2011-07-31 00:17:45 UTC
Regarding the license, the Fedora licensing page (https://fedoraproject.org/wiki/Licensing:Main#Good_Licenses) has got the following comment:
"A GPL or LGPL licensed package that lacks any statement of what version that it's licensed under in the source code/program output/accompanying docs is technically licensed under *any* version of the GPL or LGPL, not just the version in whatever COPYING file they include. Note that this is LGPLv2+, not LGPL+, because version 2 was the first version of LGPL. Note that this license was originally referred to as the GNU Library General Public License v2, but all current versions (v2.1 or newer) of the license are correctly known as the GNU Lesser General Public License."

It is confirmed by the Fedora license FAQ (https://fedoraproject.org/wiki/Licensing/FAQ#How_do_I_figure_out_what_version_of_the_GPL.2FLGPL_my_package_is_under.3F): "LGPLv2 says: If the Library does not specify a license version number, you may choose any version ever published by the Free Software Foundation."

In the case of the StdAir project, since the documentation (http://stdair.sourceforge.net/copyright.html) is pretty clear, and directly accessible from the home page (http://stdair.sourceforge.net/) and/or from the stdair-doc sub-package, the package license is correctly set as LGPLv2+ (since the copyright notice mentions LGPLv2.1 and the source code files do not further mention license versions).

Does it sound reasonable?

Comment 10 Martin Gieseking 2011-07-31 07:35:33 UTC
(In reply to comment #9)
> Does it sound reasonable?

Yes, absolutely. From the Fedora point of view, it's clear that the package is licensed under LGPLv2+ and no further action is required. However, it's good practice to add the license header to every source file as denoted in the GPL license text. If a potential third-party project takes some code files from stdair, it's always clear how the single files are licensed and who's the copyright owner even if the developer of the third-party program forgets to add a notice about stdair to his README.


Here are some more notes:

- The latest package doesn't build because of missing BR: python-devel
  http://koji.fedoraproject.org/koji/taskinfo?taskID=3240849
  There are possibly some more dependencies missing. Try to build the package
  with mock/koji to find all deps.

- As far as I can see, you can drop Requires: cmake since
  %{_datadir}/cmake/ is not used (unlike mentioned in the comment).

- Please prefer plain shell commands (rm, install, chmod, etc. ) over macros
  http://fedoraproject.org/wiki/PackagingGuidelines#Macros

- Move the find command and the following "mydocs lines" from the %check 
  to the %install section.

Comment 11 Denis Arnaud 2011-07-31 14:14:27 UTC
(In reply to comment #10)

Thanks for those additional review elements.

> - The latest package doesn't build because of missing BR: python-devel
>   http://koji.fedoraproject.org/koji/taskinfo?taskID=3240849
>   There are possibly some more dependencies missing. Try to build the package
>   with mock/koji to find all deps.

I have fixed that dependency issue (I added the requirement in CMake helpers, but forgot to add it back to the RPM specification file). mock now begins to build the package.

> - As far as I can see, you can drop Requires: cmake since
>   %{_datadir}/cmake/ is not used (unlike mentioned in the comment).

You are absolutely right. Indeed, there is no need to put anything in
%{_datadir}/cmake/ (reserved for non-CMake-aware packages), as stdair provides %{_datadir}/stdair/CMake/stdair-config*.cmake helpers (cleaner way for third party projects to use the stdair library).

> - Please prefer plain shell commands (rm, install, chmod, etc. ) over macros
>   http://fedoraproject.org/wiki/PackagingGuidelines#Macros

Ooops, I forgot that one... It has been fixed (and I will remember for other packages: thanks!).

> - Move the find command and the following "mydocs lines" from the %check 
>   to the %install section.

Ooops, it was not intentional (kind of typo). It has been fixed.

(In reply to comment #4)
> 
> - Also, it's not necessary to wrap the BuildArch field with conditionals.
> Just use plain BuildArch field.

I have left the macro around BuildArch because, if I remember correctly, it led to some issue with EPEL <= 5 (for which I intend to deliver stdair as well). More specifically, rpm, on those distributions, is/was not able to build/deliver multi-architecture packages.

========

I have now a more tricky issue with CMake and the (HTML/PDF) documentation generation. When compiling with parallel flag until '-j4', everything is fine. But above (e.g., '-j6'), there is a dependency issue with the Latex-related target (PDF reference manual generation). I have to look more thoroughly and to try a few target inter-dependency mechanisms...

Comment 12 Denis Arnaud 2011-07-31 18:53:49 UTC
Spec URL: http://denisarnaud.fedorapeople.org/stdair/stdair-0.36.1-1.spec
SRPM URL:
https://downloads.sourceforge.net/project/stdair/fedora_15/stdair-0.36.1-1.fc15.src.rpm
===============================================

The issue with the HTML/PDF documentation generation has been solved (thanks to the add_custom_command() macro to accept several output files: http://cmake.org/cmake/help/cmake2.6docs.html#command:add_custom_command). I had therefore to release a new version, namely 0.36.1.

Comment 13 Martin Gieseking 2011-08-01 13:48:20 UTC
> (In reply to comment #10)
> I have left the macro around BuildArch because, if I remember correctly, it led
> to some issue with EPEL <= 5 (for which I intend to deliver stdair as well).
> More specifically, rpm, on those distributions, is/was not able to
> build/deliver multi-architecture packages.

That's OK.


Here come the full review. The package is almost ready, and there's just one issue left:

The $RPM_OPT_FLAGS are not honored, i.e. the Makefiles don't seem to evaluate the environment variables CFLAGS/CXXFLAGS set by %cmake.
Please ensure that gcc/g++ is called with the $RPM_OPT_FLAGS.


$ rpmlint /var/lib/mock/fedora-15-i386/result/*.rpm
5 packages and 0 specfiles checked; 0 errors, 0 warnings.

---------------------------------
key:

[+] OK
[.] OK, not applicable
[X] needs work
---------------------------------

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license.
    - LGPLv2+ according to man/stdair.doc
  
[+] MUST: The License field in the package spec file must match the actual license.
[+] MUST: The file containing the text of the license(s) for the package must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source.
    $ md5sum stdair-0.36.1.tar.bz2*
    ed5dd27c0b51034e3edf9eaa252c56a6  stdair-0.36.1.tar.bz2
    ed5dd27c0b51034e3edf9eaa252c56a6  stdair-0.36.1.tar.bz2.1

[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
    koji scratch build:
    https://koji.fedoraproject.org/koji/taskinfo?taskID=3242678

[.] MUST: If the package does not successfully compile, build or work on an architecture,...
[+] MUST: All build dependencies must be listed in BuildRequires.
[X] MUST: When compiling C, C++, or Fortran files, %{optflags} must be applied.
[.] MUST: The spec file MUST handle locales properly.
[+] MUST: Packages storing shared library files (not just symlinks) must call ldconfig in %post and %postun.
[+] MUST: Packages must NOT bundle copies of system libraries.
[.] MUST: If the package is designed to be relocatable, ...
[+] MUST: A package must own all directories that it creates. 
[+] MUST: A Fedora package must not list a file more than once in %files.
[+] MUST: Permissions on files must be set properly.
[+] MUST: Each package must consistently use macros.
[+] MUST: The package must contain code, or permissable content.
[+] MUST: Large documentation files must go in a -doc subpackage.
[+] MUST: Files in %doc must not affect the runtime of the application.
[+] MUST: Header files must be in a -devel package.
[.] MUST: Static libraries must be in a -static package.
[+] MUST: If a package contains library files with a suffix, then .so files (without suffix) must go in a -devel package.
[+] MUST: devel packages must require the base package using a fully versioned dependency.
[+] MUST: Packages must NOT contain any .la libtool archives.
[.] MUST: Packages containing GUI applications must include a %{name}.desktop file.
[+] MUST: Packages must not own files or directories already owned by other packages.
[+] MUST: All filenames in rpm packages must be valid UTF-8.

EPEL <= 5 only:
[+] MUST: The spec file must contain a valid BuildRoot field.
[+] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot}.
[+] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot}.
[+] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'


[.] SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
[+] SHOULD: The reviewer should test that the package builds in mock.
[+] SHOULD: The package should compile and build into binary rpms on all supported architectures.
[+] SHOULD: The reviewer should test that the package functions as described.
[+] SHOULD: If scriptlets are used, those scriptlets must be sane.
[.] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
[+] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg.
[.] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin ...
[+] SHOULD: Your package should contain man pages for binaries/scripts.
(In reply to comment #11)

Comment 14 Denis Arnaud 2011-08-01 22:54:40 UTC
Spec URL: http://denisarnaud.fedorapeople.org/stdair/stdair-0.36.2-1.spec
SRPM URL:
https://downloads.sourceforge.net/project/stdair/fedora_15/stdair-0.36.2-1.fc15.src.rpm
===============================================

The stdair CMake build system now takes into account the C++ compilation flags (CXXFLAGS, $RPM_OPT_FLAGS).

===============================================
Thanks again for that thorough review!

Comment 15 Martin Gieseking 2011-08-02 10:39:33 UTC
(In reply to comment #14)
> Thanks again for that thorough review!

You're welcome. :)

The package looks fine now and is ready for check-in.

----------------
Package APPROVED
----------------

Comment 16 Denis Arnaud 2011-08-02 14:15:07 UTC
New Package SCM Request
=======================
Package Name: stdair
Short Description: C++ Standard Airline IT Object Library
Owners: denisarnaud
Branches: f14 f15 el4 el5 el6
InitialCC: denisarnaud

Comment 17 Gwyn Ciesla 2011-08-02 14:39:26 UTC
Git done (by process-git-requests).

Comment 18 Gwyn Ciesla 2011-08-02 14:41:53 UTC
I missed it, but you probably want to submit a package change request to add an f16 branch.

Comment 19 Denis Arnaud 2011-08-02 15:31:42 UTC
Package Change Request
======================
Package Name: stdair
New Branches: f16
Owners: denisarnaud

Comment 20 Gwyn Ciesla 2011-08-02 15:42:14 UTC
Git done (by process-git-requests).

Comment 21 Fedora Update System 2011-08-02 18:12:37 UTC
stdair-0.36.2-1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/stdair-0.36.2-1.fc16

Comment 22 Fedora Update System 2011-08-02 18:13:57 UTC
stdair-0.36.2-1.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/stdair-0.36.2-1.fc15

Comment 23 Fedora Update System 2011-08-02 18:15:06 UTC
stdair-0.36.2-1.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/stdair-0.36.2-1.fc14

Comment 24 Fedora Update System 2011-08-02 18:38:48 UTC
stdair-0.36.2-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/stdair-0.36.2-1.el6

Comment 25 Fedora Update System 2011-08-02 20:25:12 UTC
stdair-0.36.2-1.el6 has been pushed to the Fedora EPEL 6 testing repository.

Comment 26 Fedora Update System 2011-08-11 23:28:02 UTC
stdair-0.36.2-1.fc15 has been pushed to the Fedora 15 stable repository.

Comment 27 Fedora Update System 2011-08-11 23:32:51 UTC
stdair-0.36.2-1.fc14 has been pushed to the Fedora 14 stable repository.

Comment 28 Fedora Update System 2011-08-15 17:35:11 UTC
stdair-0.38.0-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/stdair-0.38.0-1.el6

Comment 29 Fedora Update System 2011-08-22 15:25:29 UTC
stdair-0.36.2-1.fc16 has been pushed to the Fedora 16 stable repository.

Comment 30 Fedora Update System 2011-08-31 22:56:13 UTC
stdair-0.38.0-1.el6 has been pushed to the Fedora EPEL 6 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 31 Fedora Update System 2011-10-15 18:21:32 UTC
stdair-0.38.0-2.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/stdair-0.38.0-2.el6

Comment 32 Fedora Update System 2011-10-18 15:15:12 UTC
stdair-0.43.1-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/stdair-0.43.1-1.el6

Comment 33 Fedora Update System 2011-11-03 00:15:26 UTC
travelccm-0.5.2-1.el6,airrac-0.2.1-2.el6,stdair-0.44.0-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/travelccm-0.5.2-1.el6,airrac-0.2.1-2.el6,stdair-0.44.0-1.el6

Comment 34 Fedora Update System 2011-11-25 02:07:34 UTC
rmol-0.25.2-1.el6, travelccm-0.5.2-1.el6, airrac-0.2.1-2.el6, stdair-0.44.0-1.el6 has been pushed to the Fedora EPEL 6 stable repository.  If problems still persist, please make note of it in this bug report.


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