Bug 890772

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

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.

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.

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!) :)

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

Still interested?

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

(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

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?

------
Spec URL: http://denisarnaud.fedorapeople.org/sim/tvlsim/tvlsim.spec
SRPM URL: http://denisarnaud.fedorapeople.org/sim/tvlsim/tvlsim-1.01.1-1.fc30.src.rpm
------
Successful build on Rawhide (F30): https://koji.fedoraproject.org/koji/taskinfo?taskID=32099763
------

* The RPM specification file has been fully cleaned according to the latest standards, as of January 2019 (see for instance https://bugzilla.redhat.com/show_bug.cgi?id=1630837).
* rpmlint does not report any error or warning.

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time, but it seems
that the review is still being working out by you. If this is right, please
respond to this comment clearing the NEEDINFO flag and try to reach out the
submitter to proceed with the review.

If you're not interested in reviewing this ticket anymore, please clear the
fedora-review flag and reset the assignee, so that a new reviewer can take
this ticket.

Without any reply, this request will shortly be resetted.

Yes, I'm still very much interested in that review to be completed (normally, it should now have become a no-brainer, as the latest iteration cleared the concerns; see #10).

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time, but it seems
that the review is still being working out by you. If this is right, please
respond to this comment clearing the NEEDINFO flag and try to reach out the
submitter to proceed with the review.

If you're not interested in reviewing this ticket anymore, please clear the
fedora-review flag and reset the assignee, so that a new reviewer can take
this ticket.

Without any reply, this request will shortly be resetted.

(In reply to Package Review from comment #13)
> This is an automatic check from review-stats script.
> 
> This review request ticket hasn't been updated for some time, but it seems
> that the review is still being working out by you. If this is right, please
> respond to this comment clearing the NEEDINFO flag and try to reach out the
> submitter to proceed with the review.
> 
> If you're not interested in reviewing this ticket anymore, please clear the
> fedora-review flag and reset the assignee, so that a new reviewer can take
> this ticket.
> 
> Without any reply, this request will shortly be resetted.

Yes, I'm still very much interested in that review to be completed (normally, it should now have become a no-brainer, as the latest iteration cleared the concerns; see #10).

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time, but it seems
that the review is still being working out by you. If this is right, please
respond to this comment clearing the NEEDINFO flag and try to reach out the
submitter to proceed with the review.

If you're not interested in reviewing this ticket anymore, please clear the
fedora-review flag and reset the assignee, so that a new reviewer can take
this ticket.

Without any reply, this request will shortly be resetted.

This is embarrasing. I am sorry.

I think I'm ready to finish this now ;-)
It's only been 10 years in the making.

(In reply to Felix Kaechele from comment #18)
> I think I'm ready to finish this now ;-)

Thanks!

A few notes before we can continue.

The package as linked here previously currently does not build. It looks like the cmake macros need updating (e.g. %cmake instead of %cmake .; %cmake_build and %cmake_install instead of %make_build and %make_install): https://docs.fedoraproject.org/en-US/packaging-guidelines/CMake/.

Other observations, going through the spec file top to bottom:

1. L5: Please update to the latest upstream version.
2. L6: Consider using %autorelease for the Release tag: https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_release_tag
3. L14-37: Consider expressing build dependencies in pkgconfig(libname) format if the CMake backend uses pkg-config to find the library: https://docs.fedoraproject.org/en-US/packaging-guidelines/PkgConfigBuildRequires/
4. L30: Using file based BuildRequires is slow (due to needing to download and traverse the file list metadata). The package that provides /usr/bin/epstopdf also has an extra Provides: texlive-epstopdf which could be used instead.
5. L40-L69, L88-92: I recommend using https for URLs listed in the description, where applicable
6. L96: the -n parameter could be removed from the %autosetup macro if you used a different Source0 URL (using the GitHub anchor URL trick): Source0:        %{url}/archive/refs/tags/%{version}.tar.gz#/%{name}-%{version}.tar.gz
7. L114 %check section can be removed if not used/not working.
8. L120,L141: COPYING file should be in %license not %doc
9. L122: %{_bindir}/simulate is a very generic binary name and may lead to future conflicts (see https://docs.fedoraproject.org/en-US/packaging-guidelines/Conflicts/#_potential_conflicting_files). I couldn't find any other packages or projects online using this path, so this is just an FYI.
10. L146: Consider using %autochangelog (https://docs.pagure.org/fedora-infra.rpmautospec/autochangelog.html)

After these things are addressed I will give it a run with fedora-review (feel free to run it against the package yourself as I will be going through all warnings and errors it produces in the review).

Many thanks, Felix, for that excellent piece of work!

Yes, you are absolutely right, it is time to upgrade that package to the latest upstream version and to the latest standard in packaging. Many thanks for all those suggestions, which I will integrate whenever possible (probably in the next few weeks, as I'm currently quite busy at work).

And you are right about the binary name, simulate is too generic. I guess something like %{name}-simulate will be better.

* Spec URL: https://denisarnaud.fedorapeople.org/sim/tvlsim/tvlsim-1.01.6-1.spec
* SRPM URL: https://denisarnaud.fedorapeople.org/sim/tvlsim/tvlsim-1.01.6-1.fc40.src.rpm
* Succesful build: https://koji.fedoraproject.org/koji/taskinfo?taskID=107946984

Thanks for that first phase of the review and for the suggestions! All those suggestions (##1-10) have been implemented.

Succesful build with ctest: https://koji.fedoraproject.org/koji/taskinfo?taskID=107948232

Copr build:
https://copr.fedorainfracloud.org/coprs/build/6557868
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-890772-tvlsim/srpm-builds/06557868/builder-live.log.gz

Please make sure the package builds successfully at least for Fedora Rawhide.

- If the build failed for unrelated reasons (e.g. temporary network
  unavailability), please ignore it.
- If the build failed because of missing BuildRequires, please make sure they
  are listed in the "Depends On" field


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

* Spec URL: https://denisarnaud.fedorapeople.org/sim/tvlsim/tvlsim-1.01.6-1.spec
* SRPM URL: https://denisarnaud.fedorapeople.org/sim/tvlsim/tvlsim-1.01.6-1.fc40.src.rpm
* Succesful build: https://koji.fedoraproject.org/koji/taskinfo?taskID=107948232

Copr build:
https://copr.fedorainfracloud.org/coprs/build/6558134
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-890772-tvlsim/fedora-rawhide-x86_64/06558134-tvlsim/fedora-review/review.txt

Please take a look if any issues were found.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

* Spec URL: https://denisarnaud.fedorapeople.org/sim/tvlsim/tvlsim-1.01.6-1.spec
* SRPM URL: https://denisarnaud.fedorapeople.org/sim/tvlsim/tvlsim-1.01.6-1.fc40.src.rpm
* Successful Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=107987308
* Successful local build with Mock: `mock -r fedora-40-x86_64 --resultdir=mock-tvlsim-1.01.6 tvlsim-1.01.6-1.fc40.src.rpm`

Created attachment 1995208 [details]
The .spec file difference from Copr build 6558134 to 6558466

Copr build:
https://copr.fedorainfracloud.org/coprs/build/6558466
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-890772-tvlsim/fedora-rawhide-x86_64/06558466-tvlsim/fedora-review/review.txt

Please take a look if any issues were found.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Everything seems clean (and working) by now. No significant error reported by the automated review.

When you have time, Felix, you can proceed with the review then.

Thanks!

Looking good except for one minor thing (the directory ownership of /usr/share/doc/tvlsim).
When that's fixed I'm good to approve.

Package Review
==============

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

Notes:
======
- BuildRequires:  cppzmq-devel could be expressed as BuildRequires:  pkgconfig(cppzmq)
- Source contains bundled libraries, but those don't end up in the built package. This is acceptable.
- fedora-review complains "Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in tvlsim-devel" but that is not actually the case.
- "tvlsim-doc.noarch: W: files-duplicate" warnings are an artifact of the doxygen process. Not a concern.
- I didn't test functionality of the package as there is some more setup involved. The "tvlsim_simulate" binary crashes and coredumps when started without proper configuration. Just thought I'd mention it.

Issues:
=======
- 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 %license.
  Note: License file copyright.html is not marked as %license
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/LicensingGuidelines/#_license_text
 -> This is a doxygen generated file, the license text is already properly marked as %license

- Spec file name must match the spec package %{name}, in the format
  %{name}.spec.
  Note: tvlsim-1.01.6-1.spec should be tvlsim.spec
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/#_spec_file_naming
 -> False alarm

- Package must own all directories that it creates.
  Note: Directories without known owners: /usr/share/doc/tvlsim
 -> The doc subpackage should use %doc %{_docdir}/%{name} instead of %doc %{_docdir}/%{name}/html

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

C/C++:
[-]: Provides: bundled(gnulib) in place as required.
     Note: Sources not installed
[x]: Package does not contain kernel modules.
[x]: If your application is a C or C++ application you must list a
     BuildRequires against gcc, gcc-c++ or clang.
[x]: ldconfig not called in %post and %postun for Fedora 28 and later.
[x]: Package does not contain any libtool archives (.la)
[x]: Package contains no static executables.
[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]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "Unknown or generated", "GNU Lesser General Public License,
     Version 2.1", "*No copyright* GNU Library General Public License,
     Version 2.0", "FSF All Permissive License", "GNU Lesser General Public
     License v2.1 or later", "Boost Software License 1.0", "GNU General
     Public License v2.0 or later [obsolete FSF postal address (Temple
     Place)]", "MIT License", "JSON License and/or MIT License", "zlib
     License", "GNU General Public License, Version 2", "*No copyright* MIT
     License". 2288 files have unknown license. Detailed output of
     licensecheck in
     /home/felix/PackageReview/tvlsim/890772-tvlsim-1.01.6-1/licensecheck.txt
  -> License of files ending up in the package are accurately represented
[x]: License file installed when any subpackage combination is installed.
[!]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/share/doc/tvlsim
[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.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (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]: 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]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[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).
[x]: The License field must be a valid SPDX expression.
[x]: Package requires other packages for directories it uses.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[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]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: File names are valid UTF-8.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 9309 bytes in 4 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

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

Generic:
[x]: 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).
[x]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in tvlsim-
     devel
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

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

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


Rpmlint
-------
Checking: tvlsim-1.01.6-1.fc40.x86_64.rpm
          tvlsim-devel-1.01.6-1.fc40.x86_64.rpm
          tvlsim-doc-1.01.6-1.fc40.noarch.rpm
          tvlsim-debuginfo-1.01.6-1.fc40.x86_64.rpm
          tvlsim-debugsource-1.01.6-1.fc40.x86_64.rpm
          tvlsim-1.01.6-1.fc40.src.rpm
============================================================================================================= rpmlint session starts =============================================================================================================
rpmlint: 2.4.0
configuration:
    /usr/lib/python3.12/site-packages/rpmlint/configdefaults.toml
    /etc/xdg/rpmlint/fedora-legacy-licenses.toml
    /etc/xdg/rpmlint/fedora-spdx-licenses.toml
    /etc/xdg/rpmlint/fedora.toml
    /etc/xdg/rpmlint/scoring.toml
    /etc/xdg/rpmlint/users-groups.toml
    /etc/xdg/rpmlint/warn-on-functions.toml
rpmlintrc: [PosixPath('/tmp/tmpuzh73jow')]
checks: 31, packages: 6

tvlsim-doc.noarch: W: files-duplicate /usr/share/doc/tvlsim/html/search/functions_6.js /usr/share/doc/tvlsim/html/search/all_12.js
tvlsim-doc.noarch: W: files-duplicate /usr/share/doc/tvlsim/html/search/functions_10.js /usr/share/doc/tvlsim/html/search/all_21.js
tvlsim-doc.noarch: W: files-duplicate /usr/share/doc/tvlsim/html/search/variables_0.js /usr/share/doc/tvlsim/html/search/all_8.js
============================================================================== 6 packages and 0 specfiles checked; 0 errors, 3 warnings, 0 badness; has taken 1.7 s ==============================================================================




Rpmlint (debuginfo)
-------------------
Checking: tvlsim-debuginfo-1.01.6-1.fc40.x86_64.rpm
============================================================================================================= rpmlint session starts =============================================================================================================
rpmlint: 2.4.0
configuration:
    /usr/lib/python3.12/site-packages/rpmlint/configdefaults.toml
    /etc/xdg/rpmlint/fedora-legacy-licenses.toml
    /etc/xdg/rpmlint/fedora-spdx-licenses.toml
    /etc/xdg/rpmlint/fedora.toml
    /etc/xdg/rpmlint/scoring.toml
    /etc/xdg/rpmlint/users-groups.toml
    /etc/xdg/rpmlint/warn-on-functions.toml
rpmlintrc: [PosixPath('/tmp/tmp94mmkgrm')]
checks: 31, packages: 1

============================================================================== 1 packages and 0 specfiles checked; 0 errors, 0 warnings, 0 badness; has taken 0.5 s ==============================================================================





Rpmlint (installed packages)
----------------------------
============================ rpmlint session starts ============================
rpmlint: 2.4.0
configuration:
    /usr/lib/python3.12/site-packages/rpmlint/configdefaults.toml
    /etc/xdg/rpmlint/fedora-legacy-licenses.toml
    /etc/xdg/rpmlint/fedora-spdx-licenses.toml
    /etc/xdg/rpmlint/fedora.toml
    /etc/xdg/rpmlint/scoring.toml
    /etc/xdg/rpmlint/users-groups.toml
    /etc/xdg/rpmlint/warn-on-functions.toml
checks: 31, packages: 5

tvlsim-doc.noarch: W: files-duplicate /usr/share/doc/tvlsim/html/search/functions_6.js /usr/share/doc/tvlsim/html/search/all_12.js
tvlsim-doc.noarch: W: files-duplicate /usr/share/doc/tvlsim/html/search/functions_10.js /usr/share/doc/tvlsim/html/search/all_21.js
tvlsim-doc.noarch: W: files-duplicate /usr/share/doc/tvlsim/html/search/variables_0.js /usr/share/doc/tvlsim/html/search/all_8.js
 5 packages and 0 specfiles checked; 0 errors, 3 warnings, 0 badness; has taken 1.6 s 



Source checksums
----------------
https://github.com/airsim/tvlsim/archive/refs/tags/v1.01.6.tar.gz#/tvlsim-1.01.6.tar.gz :
  CHECKSUM(SHA256) this package     : 298d629b07ce5f1d9a16745bc37c4eddbdb44ab2e9b314288498931657002014
  CHECKSUM(SHA256) upstream package : 298d629b07ce5f1d9a16745bc37c4eddbdb44ab2e9b314288498931657002014


Requires
--------
tvlsim (rpmlib, GLIBC filtered):
    libboost_program_options.so.1.81.0()(64bit)
    libc.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgcc_s.so.1(GCC_3.3.1)(64bit)
    libreadline.so.8()(64bit)
    libsevmgr.so.1.00()(64bit)
    libsimcrs.so.1.01()(64bit)
    libsoci_core.so.4.0()(64bit)
    libstdair.so.1.00()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libtrademgen.so.1.00()(64bit)
    libtravelccm.so.1.00()(64bit)
    libtvlsim.so.1.01()(64bit)
    libzmq.so.5()(64bit)
    rtld(GNU_HASH)

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

tvlsim-doc (rpmlib, GLIBC filtered):

tvlsim-debuginfo (rpmlib, GLIBC filtered):

tvlsim-debugsource (rpmlib, GLIBC filtered):



Provides
--------
tvlsim:
    libtvlsim.so.1.01()(64bit)
    tvlsim
    tvlsim(x86-64)

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

tvlsim-doc:
    tvlsim-doc

tvlsim-debuginfo:
    debuginfo(build-id)
    libtvlsim.so.1.01.6-1.01.6-1.fc40.x86_64.debug()(64bit)
    tvlsim-debuginfo
    tvlsim-debuginfo(x86-64)

tvlsim-debugsource:
    tvlsim-debugsource
    tvlsim-debugsource(x86-64)



Generated by fedora-review 0.10.0 (e79b66b) last change: 2023-07-24
Command line :/usr/bin/fedora-review -b 890772
Buildroot used: fedora-rawhide-x86_64
Active plugins: Shell-api, C/C++, Generic
Disabled plugins: Perl, Java, Python, Haskell, SugarActivity, PHP, fonts, R, Ocaml
Disabled flags: EXARCH, EPEL6, EPEL7, DISTTAG, BATCH

Thanks for the review!

The files (the RPM specification, as well as the source RPM) have been updated, fixing the documentation directory ownership and build-requirement for ZeroMQ:
* Spec URL: https://denisarnaud.fedorapeople.org/sim/tvlsim/tvlsim-1.01.6-1.spec
* SRPM URL: https://denisarnaud.fedorapeople.org/sim/tvlsim/tvlsim-1.01.6-1.fc40.src.rpm
* Successful Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=108661026

We should be all set by now, then :)

Many thanks again for your continuous, and instrumental, contribution!

Created attachment 1997657 [details]
The .spec file difference from Copr build 6558466 to 6608096

Copr build:
https://copr.fedorainfracloud.org/coprs/build/6608096
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-890772-tvlsim/fedora-rawhide-x86_64/06608096-tvlsim/fedora-review/review.txt

Please take a look if any issues were found.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Thanks for your patience, Denis. It's only been nearly 11 years in the making ;-)

With the latest revision I'm happy to report:

[x]: Package must own all directories that it creates.

And with that, the package is approved.

If we ever meet I owe you a beverage for keeping you waiting this long. Sorry again.

The Pagure repository was created at https://src.fedoraproject.org/rpms/tvlsim

FEDORA-2023-7ef844ecf4 has been submitted as an update to Fedora 39. https://bodhi.fedoraproject.org/updates/FEDORA-2023-7ef844ecf4

FEDORA-2023-77064a9bee has been submitted as an update to Fedora 38. https://bodhi.fedoraproject.org/updates/FEDORA-2023-77064a9bee

FEDORA-EPEL-2023-71bf8af3bf has been submitted as an update to Fedora EPEL 9. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2023-71bf8af3bf

FEDORA-2023-77064a9bee has been pushed to the Fedora 38 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2023-77064a9bee \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2023-77064a9bee

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-EPEL-2023-71bf8af3bf has been pushed to the Fedora EPEL 9 testing repository.

You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2023-71bf8af3bf

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2023-7ef844ecf4 has been pushed to the Fedora 39 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2023-7ef844ecf4 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2023-7ef844ecf4

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-EPEL-2023-ae16de6e91 has been submitted as an update to Fedora EPEL 8. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2023-ae16de6e91

FEDORA-EPEL-2023-ae16de6e91 has been pushed to the Fedora EPEL 8 testing repository.

You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2023-ae16de6e91

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2023-7ef844ecf4 has been pushed to the Fedora 39 stable repository.
If problem still persists, please make note of it in this bug report.

FEDORA-2023-77064a9bee has been pushed to the Fedora 38 stable repository.
If problem still persists, please make note of it in this bug report.

FEDORA-EPEL-2023-71bf8af3bf has been pushed to the Fedora EPEL 9 stable repository.
If problem still persists, please make note of it in this bug report.

FEDORA-EPEL-2023-ae16de6e91 has been pushed to the Fedora EPEL 8 stable repository.
If problem still persists, please make note of it in this bug report.