This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 890772 - Review Request: tvlsim - Travel Market Simulator
Review Request: tvlsim - Travel Market Simulator
Status: ASSIGNED
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Felix Kaechele
Fedora Extras Quality Assurance
:
Depends On: 732218 760594 781775
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-29 16:48 EST by Denis Arnaud
Modified: 2015-04-18 17:37 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
felix: fedora‑review?


Attachments (Terms of Use)

  None (edit)
Description Denis Arnaud 2012-12-29 16:48:18 EST
Spec URL: http://denisarnaud.fedorapeople.org/sim/tvlsim/tvlsim.spec
SRPM URL: http://denisarnaud.fedorapeople.org/sim/tvlsim/tvlsim-1.00.0-1.fc17.src.rpm
Description:
The Travel Market Simulator project aims at providing reference implementation,
mainly in C++, of a travel market simulator, focusing on revenue management (RM)
for airlines. It is intended to be used for applied research activities only:
it is by no way intended to be used by production systems. It is a new breed of
software and aims to become the new generation PODS (http://podsresearch.com/),
which was instrumental in the inception of the Travel Market Simulator project.
The Travel Market Simulator can used in either batch or hosted mode. It is
the main component of the Travel Market Simulator:
http://www.travel-market-simulator

Fedora Account System Username: denisarnaud
Comment 1 Felix Kaechele 2013-08-03 11:32:08 EDT
Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: ldconfig called in %post and %postun if required.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[x]: Each %files section contains %defattr if rpm < 4.4
     Note: %defattr present but not needed
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[-]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[!]: Fully versioned dependency in subpackages, if present.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in tvlsim-doc
[x]: Package complies to the Packaging Guidelines
[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "GPL (v2 or later) (with incorrect FSF address)", "BSL (v1.0)", "Unknown
     or generated". 158 files have unknown license. Detailed output of
     licensecheck in /home/felix/Downloads/890772-tvlsim/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[-]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 40960 bytes in 5 files.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).

===== SHOULD items =====

Generic:
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[!]: Buildroot is not present
     Note: Buildroot: present but not needed
[!]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: %clean present but not required
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[!]: Package functions as described.
     Note: Segfaults on start
     terminate called after throwing an instance of 'stdair::SQLDatabaseConnectionImpossibleException'
       what():  Error while opening a connection to database: Can't connect to local MySQL server through socket '/var/lib/mysql/mysql.sock' (2)
     Database parameters used: db=sim_tvlsim user=tvlsim port=3306 host=localhost
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Scriptlets must be sane, if used.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[!]: Package should compile and build into binary rpms on all supported
     architectures.
     Note: http://koji.fedoraproject.org/koji/taskinfo?taskID=5722841
           looks like some packages need rebuilding against new boost in rawhide
[!]: %check is present and all tests pass.
     Note: Package ships tests, consider running them.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make.
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: tvlsim-1.00.0-1.fc19.x86_64.rpm
          tvlsim-devel-1.00.0-1.fc19.x86_64.rpm
          tvlsim-doc-1.00.0-1.fc19.noarch.rpm
tvlsim.x86_64: W: spelling-error %description -l en_US traveller -> traveler, traveled, traversal
tvlsim.x86_64: W: spelling-error %description -l en_US modelling -> modeling, model ling, model-ling
3 packages and 0 specfiles checked; 0 errors, 2 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint tvlsim-doc tvlsim tvlsim-devel
tvlsim.x86_64: W: spelling-error %description -l en_US traveller -> traveler, traveled, traversal
tvlsim.x86_64: W: spelling-error %description -l en_US modelling -> modeling, model ling, model-ling
tvlsim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtvlsim.so.1.00.0 /lib64/libpython2.7.so.1.0
tvlsim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtvlsim.so.1.00.0 /lib64/libboost_regex-mt.so.1.53.0
tvlsim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtvlsim.so.1.00.0 /lib64/libboost_iostreams-mt.so.1.53.0
tvlsim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtvlsim.so.1.00.0 /lib64/libboost_serialization-mt.so.1.53.0
tvlsim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtvlsim.so.1.00.0 /lib64/libboost_filesystem-mt.so.1.53.0
tvlsim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtvlsim.so.1.00.0 /lib64/libboost_python-mt.so.1.53.0
tvlsim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtvlsim.so.1.00.0 /usr/lib64/mysql/libmysqlclient.so.18
tvlsim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtvlsim.so.1.00.0 /lib64/libsoci_mysql.so.3.2
tvlsim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtvlsim.so.1.00.0 /lib64/libzmq.so.1
tvlsim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtvlsim.so.1.00.0 /lib64/libreadline.so.6
tvlsim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtvlsim.so.1.00.0 /lib64/libncurses.so.5
tvlsim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtvlsim.so.1.00.0 /lib64/libtinfo.so.5
tvlsim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtvlsim.so.1.00.0 /lib64/libstdairuicl.so.1.00
tvlsim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtvlsim.so.1.00.0 /lib64/libairsched.so.1.00
tvlsim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtvlsim.so.1.00.0 /lib64/libairrac.so.1.00
tvlsim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtvlsim.so.1.00.0 /lib64/librmol.so.1.00
tvlsim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtvlsim.so.1.00.0 /lib64/libairinv.so.1.00
tvlsim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtvlsim.so.1.00.0 /lib64/libsimfqt.so.1.00
3 packages and 0 specfiles checked; 0 errors, 20 warnings.
# echo 'rpmlint-done:'



Requires
--------
tvlsim-doc (rpmlib, GLIBC filtered):

tvlsim (rpmlib, GLIBC filtered):
    /sbin/ldconfig
    libairinv.so.1.00()(64bit)
    libairrac.so.1.00()(64bit)
    libairsched.so.1.00()(64bit)
    libboost_date_time-mt.so.1.53.0()(64bit)
    libboost_filesystem-mt.so.1.53.0()(64bit)
    libboost_iostreams-mt.so.1.53.0()(64bit)
    libboost_program_options-mt.so.1.53.0()(64bit)
    libboost_python-mt.so.1.53.0()(64bit)
    libboost_regex-mt.so.1.53.0()(64bit)
    libboost_serialization-mt.so.1.53.0()(64bit)
    libc.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libm.so.6()(64bit)
    libmysqlclient.so.18()(64bit)
    libncurses.so.5()(64bit)
    libpthread.so.0()(64bit)
    libpython2.7.so.1.0()(64bit)
    libreadline.so.6()(64bit)
    librmol.so.1.00()(64bit)
    libsevmgr.so.1.00()(64bit)
    libsimcrs.so.1.00()(64bit)
    libsimfqt.so.1.00()(64bit)
    libsoci_core.so.3.2()(64bit)
    libsoci_mysql.so.3.2()(64bit)
    libstdair.so.1.00()(64bit)
    libstdairuicl.so.1.00()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libstdc++.so.6(CXXABI_1.3.1)(64bit)
    libtinfo.so.5()(64bit)
    libtrademgen.so.1.00()(64bit)
    libtravelccm.so.1.00()(64bit)
    libtvlsim.so.1.00()(64bit)
    libzmq.so.1()(64bit)
    rtld(GNU_HASH)

tvlsim-devel (rpmlib, GLIBC filtered):
    /bin/sh
    /usr/bin/pkg-config
    libtvlsim.so.1.00()(64bit)
    pkgconfig
    tvlsim(x86-64)



Provides
--------
tvlsim-doc:
    tvlsim-doc

tvlsim:
    libtvlsim.so.1.00()(64bit)
    tvlsim
    tvlsim(x86-64)

tvlsim-devel:
    pkgconfig(tvlsim)
    tvlsim-devel
    tvlsim-devel(x86-64)



Source checksums
----------------
http://downloads.sourceforge.net/tvlsim/tvlsim-1.00.0.tar.bz2 :
  CHECKSUM(SHA256) this package     : e2eec481359c2ce8780e27f8ba4a8ea5af31db3245c1138a49057b7e57ae42ec
  CHECKSUM(SHA256) upstream package : e2eec481359c2ce8780e27f8ba4a8ea5af31db3245c1138a49057b7e57ae42ec


Generated by fedora-review 0.4.1 (b2e211f) last change: 2013-04-29
Buildroot used: fedora-19-x86_64
Command line :/usr/bin/fedora-review -b 890772
Comment 2 Denis Arnaud 2013-08-04 21:08:49 EDT
Many thanks for that review!

Following are the updated RPM specification file and source RPM:
------
Spec URL: http://denisarnaud.fedorapeople.org/sim/tvlsim/tvlsim.spec
SRPM URL: http://denisarnaud.fedorapeople.org/sim/tvlsim/tvlsim-1.00.0-2.fc19.src.rpm
=====


Specific answers to the issues spotted by the review
----------------------------------------------------
1. Documentation sub-package
> [!]: Fully versioned dependency in subpackages, if present.
>      Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in tvlsim-doc

Note that the absence of "Requires:" was intentional, as the documentation (-doc) sub-package may be installed independently of the main package. Hence, someome may first install the -doc sub-package, check that it corresponds to his/her expectations, and only then installing the main package if deemed necessary.
=> It should be a [x]


2. License
> [!]: License field in the package spec file matches the actual license.
>      Note: Checking patched sources after %prep for licenses. Licenses found:
>      "GPL (v2 or later) (with incorrect FSF address)", "BSL (v1.0)", "Unknown
>      or generated". 158 files have unknown license. Detailed output of
>      licensecheck in /home/felix/Downloads/890772-tvlsim/licensecheck.txt

The license of TvlSim is LGPL v2.1, as stated by the COPYING file on the GitHub repository: http://github.com/airsim/tvlsim/blob/trunk/COPYING
That file is the verbatim of the LGPL v2.1 text: http://www.gnu.org/licenses/lgpl-2.1.html#SEC1
Since the source files do not mention any license, the license of the whole project applies for each of the source files.
=> It should be a [x]


3. BuildRoot
> [!]: Buildroot is not present
>      Note: Buildroot: present but not needed
> [!]: Package has no %clean section with rm -rf %{buildroot} (or
>      $RPM_BUILD_ROOT)
>      Note: %clean present but not required

BuildRoot is present because I intend to package TvlSim for EPEL (5 and 6).


4. Segfault on start
> [!]: Package functions as described.
>      Note: Segfaults on start
>      terminate called after throwing an instance of 'stdair::SQLDatabaseConnectionImpossibleException'
>        what():  Error while opening a connection to database: Can't connect to local MySQL server through socket '/var/lib/mysql/mysql.sock' (2)
>      Database parameters used: db=sim_tvlsim user=tvlsim port=3306 host=localhost

4.1. 'tvlsim' executable
That executable works out of the box, without any further configuration. For instance:
$ tvlsim
tvlsim > run
tvlsim > quit
$ less tvlsim.log 

4.2. 'simulate' executable
That executable does not work out of the box. It needs a MySQL database server. Then, the StdAir package contains helper scripts to create a database user, namely 'tvlsim', as well as a database, namely 'sim_tvlsim'. Some data need to be loaded into a specific table, namely 'airlines'. Following is the sequence of commands allowing to initialise the MySQL database correctly:
$ /usr/libexec/stdair/create_tvlsim_user.sh # Then, enter the credentials of an admin MySQL account. Launch with the '-h' option to see how to change the hostname and/or port.
$ /usr/libexec/stdair/create_tvlsim_db.sh # Then, enter the credentials of an admin MySQL account. Launch with the '-h' option to see how to change the hostname and/or port.
$ /usr/libexec/stdair/load_tvlsim_data.sh # Launch with the '-h' option to see how to change the hostname and/or port.
$ simulate
$ less simulate.log

4.3. 'TvlSimServer' executable
Same as for the 'simulate' executable.
A command-line (test) client exists in Python (https://github.com/airsim/tvlsim/blob/trunk/appserver/django/tvlsimClient.py), but it is not installed by the upstream installation scripts.
$ TvlSimServer &
$ wget https://raw.github.com/airsim/tvlsim/trunk/appserver/django/tvlsimClient.py
$ python tvlsimClient.py
$ kill %1
$ less tvlsimServer.log
Note that a Django-based Web application server exists, but it has not been packaged yet.

=> It should be a [x]


5. Successful build on Koji for Rawhide
> [!]: Package should compile and build into binary rpms on all supported
     architectures.
>      Note: http://koji.fedoraproject.org/koji/taskinfo?taskID=5722841
           looks like some packages need rebuilding against new boost in rawhide

The mass rebuild (https://fedoraproject.org/wiki/Fedora_20_Mass_Rebuild) is still on-going. Almost all the components of the simulator (TvlSim) have been successfully rebuilt on the 'f20-rebuild' side tag (for instance, AirInv: http://koji.fedoraproject.org/koji/taskinfo?taskID=5696380), and are now relying on Boost-1.54. The build of tvlsim on the 'f20-rebuild' side tag (and, soon, on Rawhide) should therefore be successful, as it is on Fedora 18 and 19.

=> It should be a [x]


6. %check
> [!]: %check is present and all tests pass.
>      Note: Package ships tests, consider running them.

Due to the MySQL configuration dependency, some tests fail when the MySQL has not been properly configured (which is the case in the chroot environment set up by mock). Hence, the tests are de-activated.
Comment 3 Felix Kaechele 2013-08-26 09:54:29 EDT
Sorry for the delay.

Your explanations of the various issues are reasonable.
I rebuilt the package on koji to confirm it now builds correctly: http://koji.fedoraproject.org/koji/taskinfo?taskID=5854028

Some hints which are probably nice to have but not mandatory:
Try contacting upstream to add header information to the source files (i.e. add something like "this source file belongs to the tvlsim project and is licensed under GPLv2+".
Also segfaulting, in my opinion, is not a nice way to tell the user to set up MySQL correctly ;) Maybe upstream should add some better error handling here? 

One thing I noticed only now is that your spec file uses British English spelling (see rpmlint warnings). It doesn't bother me, however the Review Guidelines have a MUST on American English spelling. See https://fedoraproject.org/wiki/Packaging:Guidelines#summary

If you fix the spelling I will approve the package.
Comment 4 Denis Arnaud 2013-08-31 16:52:05 EDT
Many thanks for that review!

Following are the updated RPM specification file and source RPM:
------
Spec URL: http://denisarnaud.fedorapeople.org/sim/tvlsim/tvlsim.spec
SRPM URL: http://denisarnaud.fedorapeople.org/sim/tvlsim/tvlsim-1.00.0-3.fc19.src.rpm
=====

(In reply to Felix Kaechele from comment #3)
> One thing I noticed only now is that your spec file uses British English
> spelling (see rpmlint warnings). It doesn't bother me, however the Review
> Guidelines have a MUST on American English spelling. See
> https://fedoraproject.org/wiki/Packaging:Guidelines#summary
> 
> If you fix the spelling I will approve the package.

Note that I had already corrected the spelling in the second version of the specification file (http://denisarnaud.fedorapeople.org/sim/tvlsim/tvlsim-1.00.0-2.spec). However, cross-reading all the texts carefully, I noticed some typos, which I fixed, hence the newer version above.

As for your other comments (e.g., regarding the license), I will tell upstream for sure (as I am upstream!) :)
Comment 5 Felix Kaechele 2013-09-04 04:22:48 EDT
I was thinking about the rpmlint warning regarding unused direct library dependencies.
Those can be fixed by passing "-Wl,--as-needed" to the LDFLAGS from cmake, I guess.
Maybe this is something you want to consider? I haven't checked if this breaks anything (as the output actually looks like false positives).
Comment 6 Felix Kaechele 2014-08-26 16:28:42 EDT
Still interested?
Comment 7 Denis Arnaud 2014-08-29 13:19:12 EDT
Yes, for sure, I am still very much interested in having that package finding its way to Fedora/CentOS/RedHat, as it is the main entry point for the whole Travel Market Simulator project (http://www.travel-market-simulator.com). Without that last package, the project is a lot less interesting...
Thanks in advance!
Kind regards
Denis
Comment 8 Christopher Meng 2014-08-30 02:35:16 EDT
(In reply to Felix Kaechele from comment #5)
> I was thinking about the rpmlint warning regarding unused direct library
> dependencies.
> Those can be fixed by passing "-Wl,--as-needed" to the LDFLAGS from cmake, I
> guess.
> Maybe this is something you want to consider? I haven't checked if this
> breaks anything (as the output actually looks like false positives).

In cmake world it's pretty common:

http://www.cmake.org/Wiki/CMake_FAQ#Why_are_libraries_linked_to_my_shared_library_included_when_something_links_to_it.3F
Comment 9 Denis Arnaud 2015-04-18 17:35:39 EDT
Spec URL: http://denisarnaud.fedorapeople.org/sim/tvlsim/tvlsim.spec
SRPM URL: http://denisarnaud.fedorapeople.org/sim/tvlsim/tvlsim-1.00.0-4.fc20.src.rpm
-----------------
Thanks Christopher for your suggestion!

Well, I (as upstream) wanted to give that LDFLAGS configuration a try... but the head/trunk of source code now relies on AirTSP, rather than on AirSched; so, http://bugzilla.redhat.com/show_bug.cgi?id=972431 is in the way.

Another option, in the meantime, would be to alter the LDFLAGS directly from the tvlsim.spec file. But it is safer to do it upstream; an issue has been created upstream: http://github.com/airsim/tvlsim/issues/1.

----------
Successful builds:
Fedora 22: http://koji.fedoraproject.org/koji/taskinfo?taskID=9508938
Fedora 21: http://koji.fedoraproject.org/koji/taskinfo?taskID=9508865
------------

So, could you approve it?

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