Bug 866265 - Review Request: opentrep - C++ API for parsing travel-focused requests
Summary: Review Request: opentrep - C++ API for parsing travel-focused requests
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Robert-André Mauchin 🐧
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-10-14 23:42 UTC by Denis Arnaud
Modified: 2019-03-11 20:55 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2019-03-04 00:51:24 UTC
Type: ---
Embargoed:
zebob.m: fedora-review+


Attachments (Terms of Use)

Description Denis Arnaud 2012-10-14 23:42:07 UTC
Spec URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep.spec
SRPM URL: <srpm info here>
Description: http://denisarnaud.fedorapeople.org/opentrep/opentrep-0.5.0-1.fc17.src.rpm
Fedora Account System Username: denisarnaud

Description:
OpenTrep aims at providing a clean API, and the corresponding C++
implementation, for parsing travel-focused requests (e.g.,
"washington dc beijing monday a/r +aa -ua 1 week 2 adults 1 dog").

OpenTrep uses Xapian (http://www.xapian.org) for the Information Retrieval part,
on freely available travel-related data (e.g., country names and codes,
city names and codes, airline names and codes, etc.).

OpenTrep exposes a simple, clean and object-oriented, API. For instance,
the static Parse() method takes, as input, a string containing the travel
request, and yields, as output, the list of the recognised terms as well as
their corresponding types. As an example, the travel request
"washington dc beijing monday a/r +aa -ua 1 week 2 adults 1 dog" would give
the following list:
  * Origin airport: Washington, DC, USA
  * Destination airport: Beijing, China
  * Date of travel: next Monday
  * Date of return: 1 week after next Monday
  * Preferred airline: American Airlines; non-preferred airline: United Airlines
  * Number of travellers: 2 adults and a dog

The output can then be used by other systems, for instance to book the
corresponding travel or to visualise it on a map and calendar and to
share it with others.

OpenTrep makes an extensive use of existing open-source libraries for
increased functionality, speed and accuracy. In particular the
Boost (C++ Standard Extensions: http://www.boost.org) library is used.

Comment 1 Volker Fröhlich 2012-10-22 21:17:46 UTC
Better make it "1*" and "3*" for the man pages instead of "1.*".

defattr is no longer necessary -- also not for EPEL 5.

You simplify

%dir %{_datadir}/%{name}
%dir %{_datadir}/%{name}/data
%dir %{_datadir}/%{name}/data/por
%{_datadir}/%{name}/data/por/ori_por_public.csv

to

%{_datadir}/%{name}

You seem to be missing ICU as BR, because the build fails for me.

Comment 2 Denis Arnaud 2012-10-24 23:10:30 UTC
Thanks Volker for the review. I will have a look at it shortly and publish the amended versions of the package.

Comment 3 Denis Arnaud 2012-10-25 21:11:48 UTC
[For later reference, I just add the corresponding URLs of the packaging guidelines]

Spec URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep.spec
SRPM URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep-0.5.0-2.fc17.src.rpm


(In reply to comment #1)
> Better make it "1*" and "3*" for the man pages instead of "1.*".

Reference: http://fedoraproject.org/wiki/Packaging:Guidelines#Man_pages

If you do not mind, I prefer keeping "1.*"/"3.*", as it implies that the manual pages are compressed when installed and that the non compressed version should not be installed. If I specify "1*"/"3*", and that by chance or error, the manual pages are not compressed, I will not be aware of it and the non compressed version will be installed.
But I may have missed your point; do not hesitate to elaborate.


> defattr is no longer necessary -- also not for EPEL 5.

Reference: http://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions

You are right. Done


> You simplify
> %dir %{_datadir}/%{name}
> %dir %{_datadir}/%{name}/data
> %dir %{_datadir}/%{name}/data/por
> %{_datadir}/%{name}/data/por/ori_por_public.csv
> 
> to
> 
> %{_datadir}/%{name}

Reference: http://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership
http://fedoraproject.org/wiki/Packaging:UnownedDirectories

You are right, it can be simplified. However, I did it that way on purpose, in order to install only the data file I intended to be installed. That way, if in the future other data files are added to that directory in the source tar-ball, I will be aware of it. I got my inspiration from: http://fedoraproject.org/wiki/Packaging:UnownedDirectories#Only_Including_Files


> You seem to be missing ICU as BR, because the build fails for me.

Reference: http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires

You are right. And I removed soci-mysql-devel, as the MySQL database is no longer used.

Comment 4 Michael Schwendt 2012-12-13 20:28:35 UTC
> %dir %{_datadir}/%{name}
> %dir %{_datadir}/%{name}/data
> %dir %{_datadir}/%{name}/data/por
> %{_datadir}/%{name}/data/por/ori_por_public.csv

is simple enough. [There are %files sections that are much more difficult to read with many more %dir statements and many more explicitly listed files.]

Actually, simplifying the above entries further would present a disadvantage, *if* one wants to ensure that certain files get packaged. For example,

  %{_datadir}/%{name}/

would include *anything*, even an empty directory at %{_datadir}/%{name}. Imagine you want to make sure that the file %{_datadir}/%{name}/data/por/ori_por_public.csv gets packaged (or an important header file that belongs to an API). Well, you could add a corresponding existance-test to the %check section instead of making the %files section explicit. But as Denis points out correctly, the build wouldn't fail for files in the buildroot that were installed by mistake or in a wrong path (without the packager getting a chance to double-check it). So, there is a use-case for making %files section more explicit instead of including trees to simplify them.

Comment 5 Denis Arnaud 2012-12-13 23:26:24 UTC
Thanks Michael!
The package is still waiting for an approval. So, do not hesitate to come forward :)

Comment 9 Michael Schwendt 2013-08-12 10:35:29 UTC
Amazing. It's really assigned to me already, but doesn't show up anywhere in my reviews folder.

[...]

In the build.log:

> sh: epstopdf: command not found

Many times. Adding that tool as BR doesn't seem to change anything.

  $ repoquery --whatprovides /usr/bin/epstopdf
  texlive-epstopdf-bin-3:svn18336.0-0.1.20130608_r30832.fc19.noarch
  texlive-epstopdf-bin-2:svn18336.0-22.20130427_r30134.fc19.noarch

There's a PDF in the -doc package, which contains:

  $ cat refman.pdf 
  Warning:\ the\ PDF\ reference\ manual\ (/builddir/build/BUILD/opentrep-0.5.3/doc/latex/refman.pdf)\ has\ failed\ to\ build.\ You\ can\ perform\ a\ simple\ re-build\ (make\ in\ the\ doc/latex\ sub-directory).

[...]

> opentrep-devel-0.5.3-2.fc19.x86_64.rpm 
> /bin/sh
> pkgconfig

The /bin/sh is for a shell-script:

-rwxr-xr-x  /usr/bin/opentrep-config

It contains a hardcoded libdir path and therefore is not multilib-installable.
One common way to fix it is to make the script retrieve variables from
the pkgconfig file:

  $ pkg-config --variable=libdir opentrep
  /usr/lib64

[...]

> opentrep-0.5.3-2.fc19.x86_64.rpm 
>
> -rwxr-xr-x  /usr/bin/pyopentrep

Same here (not multilib-installable) and more. It appends /usr/lib64 to sys.path in an attempt at finding libpyopentrep, but that won't work as expected. The needed *.so lib is stored in the optional -devel package:

  $ pyopentrep 
  Traceback (most recent call last):
  File "/usr/bin/pyopentrep", line 286, in <module>
      import libpyopentrep
  ImportError: No module named libpyopentrep

With this run-time requirement, it would be plausible to move libpyopentrep.so into the base package, too. Even cleaner would be to store it in Python's search path for modules, so appending to sys.path would not be needed.

Comment 10 Denis Arnaud 2013-08-12 12:16:11 UTC
Many thanks, Michael, for that (piece of) review; it is already very valuable!
I'll work on it asap, but probably not before next weekend (my day job keeps me quite busy these days).

Comment 11 Denis Arnaud 2013-08-13 01:10:42 UTC
Spec URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep.spec
SRPM URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep-0.5.3-3.fc19.src.rpm

Successful build on Koji for Rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=5810028
----------------

1. Multilib support with opentrep-config
$ opentrep-config --libs
/usr/lib64 -lopentrep
So, it now works as expected (AFAIU)


2. Documentation generation
Some Latex style packages were missing. I have added them. But there is still a tricky error related to an hyperlink spanning two pages (see, for instance, http://www.postgresql.org/message-id/9473.1296172647@sss.pgh.pa.us):
"! pdfTeX error (ext4): \pdfendlink ended up in different nesting level than \pd
fstartlink."
Nevertheless, the PDF version of the documentation is not that much important. So, for now, it is not a big deal if it is not built, installed and packaged.


3. Python module/executable
[For reference: http://fedoraproject.org/wiki/Packaging:Python]

rpmlint still reports the following warnings:
opentrep.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/libpyopentrep/libpyopentrep.so.0.5.3 libpyopentrep.so.0.5()(64bit)
opentrep.x86_64: W: devel-file-in-non-devel-package /usr/lib64/python2.7/site-packages/libpyopentrep/libpyopentrep.so


I understand that only the .so library should be in the %{python_sitearch} directory (hence, the .so should be the actual library, not a symbolic link, and the .so.0.5 library should not exist). But I may have missed something.
For instance, then, why does the second warning occur?

I have checked some other Python binding packages, such as the Xapian ones (xapian-bindings-python):
$ ls -l /usr/lib64/python2.7/site-packages/xapian/
[...]
-rwxr-xr-x  1 root root 893K Mar 24 17:34 _xapian.so*

$ file /usr/lib64/python2.7/site-packages/xapian/_xapian.so
ELF 64-bit LSB shared object, x86-64, version 1 (SYSV), dynamically linked, stripped

$ ldd /usr/lib64/python2.7/site-packages/xapian/_xapian.so 
libxapian.so.22 => /usr/lib64/libxapian.so.22

If you have some hints, they would be welcome.

Thanks!

Comment 12 Christopher Meng 2013-11-03 14:19:52 UTC
ping.

Comment 13 Denis Arnaud 2013-11-03 17:57:08 UTC
(In reply to Christopher Meng from comment #12)
> ping.

the package works, and it would make sense to release (approve) it as is. Of course, some more work would be needed to make it perfect. But, it also needs quite a lot more time and energy, which I do not have right now...

Comment 14 Christopher Meng 2014-01-05 12:07:17 UTC
(In reply to Denis Arnaud from comment #13)
> (In reply to Christopher Meng from comment #12)
> > ping.
> 
> the package works, and it would make sense to release (approve) it as is. Of
> course, some more work would be needed to make it perfect. But, it also
> needs quite a lot more time and energy, which I do not have right now...

I ping Michael months ago...

I don't this package can works fine on RHEL < 7:

-- Requires PythonLibs-2.7
-- Found PythonLibs: /usr/lib64/libpython2.7.so (Required is at least version "2.7")
-- Found PythonLibs 2.7
-- Requires Boost-1.41
-- Boost version: 1.46.0
-- Found the following Boost libraries:

So you'd better optimize the SPEC of conditional lines and remove obsoleted lines.

Comment 15 Michael Schwendt 2014-01-05 12:38:30 UTC
Sorry. I get too much mail from bugzilla, and an inflational number of "ping"s or non-verbose NEEDINFO requests is not helpful in that case either.

Comment 16 Michael Schwendt 2014-01-05 13:37:53 UTC
Re: comment 11

> Multilib support with opentrep-config

opentrep-devel.i686 and opentrep-devel.x86_64 conflict with eachother, because this script contains a hardcoded libdir definition.

There is no extra rule in the packaging guidelines for multilib conflicts in -devel packages, because they are treated like any other conflicts. Sometimes with lower priority. Sometimes with mass-filed tickets. There's even a tracker ticket for such issues in RHEL6 related repos.


> why does the second warning occur?

Because rpmlint cannot tell whether a .so file _really_ belongs into a -devel package. It's the responsibility of the packager to decide on that. If the .so file is private shared lib that's part of a Python module, certainly it must not be moved into an optional -devel package.


> opentrep-0.5.3-3.fc19.src.rpm

$ rpmls opentrep.x86_64|grep pyopentrep
-rwxr-xr-x  /usr/lib/python2.7/site-packages/opentrep/pyopentrep
drwxr-xr-x  /usr/lib64/python2.7/site-packages/libpyopentrep
lrwxrwxrwx  /usr/lib64/python2.7/site-packages/libpyopentrep/libpyopentrep.so
lrwxrwxrwx  /usr/lib64/python2.7/site-packages/libpyopentrep/libpyopentrep.so.0.5
-rwxr-xr-x  /usr/lib64/python2.7/site-packages/libpyopentrep/libpyopentrep.so.0.5.3
-rw-r--r--  /usr/share/man/man1/pyopentrep.1.gz

So, the man page covers a program, which is not available in $PATH. Trying to run it with full path:

$ /usr/lib/python2.7/site-packages/opentrep/pyopentrep
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/opentrep/pyopentrep", line 286, in <module>
    import libpyopentrep
ImportError: No module named libpyopentrep

Additionally, it's stored in /usr/lib but contains a hardcoded /usr/lib64 (see comment 9) and therefore causes a multilib conflict.


* "fedora-review -b 866265" reports one source file that is BSD 3-clause licensed and not LGPLv2+:

*No copyright* BSD (3 clause)
-----------------------------
/var/lib/mock/fedora-20-x86_64/root/builddir/build/BUILD/opentrep-0.5.3/opentrep/basic/float_utils_google.hpp

Comment 17 Denis Arnaud 2014-01-05 20:53:01 UTC
(In reply to Michael Schwendt from comment #16)

Many thanks Michael, that is very helpful. I will have a look whenever I can.

Comment 18 Denis Arnaud 2014-02-02 22:26:54 UTC
Spec URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep.spec
SRPM URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep-0.6.0-1.fc20.src.rpm

Succesful Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=6482267

Note that that version does not take into account all Michael's latest feedback yet. My point was to bring here the latest upstream updates. I will go on working on the issues mentioned above in comment #16.

Comment 19 Denis Arnaud 2014-02-03 02:11:15 UTC
Spec URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep.spec
SRPM URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep-0.6.0-1.fc20.src.rpm

Successful Koji build (for all the architectures):
http://koji.fedoraproject.org/koji/taskinfo?taskID=6482718

I have made an attempt to fix all the issues raised above by Michael:
* Occurrence of BSD license for one source file.
* Multi-lib support for the opentrep-config script.
* Multi-lib support for the Python module.

So, the package is ready again for (hopefully the end of) the review.

Thanks!

Comment 20 Denis Arnaud 2014-02-03 17:27:35 UTC
Spec URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep.spec
SRPM URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep-0.6.0-1.fc20.src.rpm

Successful Koji build (for all the architectures):
http://koji.fedoraproject.org/koji/taskinfo?taskID=6486024

Now everything appears to work as expected*.

*: Note that the Python script has got a manual page, but can only be executed from the Python site-packages directory. I do not know how to solve that issue.

Comment 21 Denis Arnaud 2014-02-09 18:58:14 UTC
Spec URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep.spec
SRPM URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep-0.6.0-1.fc20.src.rpm

Successful Koji build (for all the architectures):
http://koji.fedoraproject.org/koji/taskinfo?taskID=6509587

Now everything appears to work as expected*.

*: Note that the Python script has got a manual page, but can only be executed from the Python site-packages directory. I do not know how to solve that issue.

Comment 22 Michael Schwendt 2014-03-06 12:22:53 UTC
> %package        devel
> Requires:       pkgconfig

pkgconfig deps are automatic for Fedora and EL6


> %package        data
> Summary:        Referential data for the %{name} library
> Group:          System Environment/Libraries

Just for anyone who might wonder about this group tag in the future: It's a somewhat strange group for an optional data package, which is used by a lib but is not a lib. There are no good alternatives in /usr/share/doc/rpm/GROUP however, at most Applications/Databases, which wouldn't be better IMO.



Also just for completeness, "fedora-review -b 866265" reports 

  BSL (v1.0)
  ----------
  opentrep-0.6.0/test/parsers/full_calculator.cpp
  opentrep-0.6.0/test/parsers/parameter_parser.cpp

which only applies to test source code files.



> %package        data

It's possible to put an own "License: CC-BY-SA" tag into this subpackage.



/usr/bin/opentrep-config will still conflict, because of the line

  #libdir=@libdir@

that will contain the expanded libdir value. Easy to fix in the opentrep-config.in template.



The Provides for the Python module libpyopentrep.so.0.6 are [potentially] problematic: 
https://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering

  $ rpm -qp --provides opentrep-0.6.0-1.fc20.x86_64.rpm 
  libopentrep.so.0.6()(64bit)
* libpyopentrep.so.0.6()(64bit)
  opentrep = 0.6.0-1.fc20
  opentrep(x86-64) = 0.6.0-1.fc20

Not a blocker.

  $ repoquery --whatprovides libpyopen\*
  $

But notice:

  $ repoquery --whatprovides libpy\*|wc -l
  70


> Note that the Python script has got a manual page,
> but can only be executed from the Python site-packages directory.
> I do not know how to solve that issue.

It could be moved into /usr/bin after deleting the sys.path.append lines which are commented out anyway. Its Python module is stored in Python search path.


$ /usr/lib64/python2.7/site-packages/libpyopentrep/pyopentrep
Traceback (most recent call last):
  File "/usr/lib64/python2.7/site-packages/libpyopentrep/pyopentrep", line 7, in <module>
    import Travel_pb2
  File "/usr/lib64/python2.7/site-packages/libpyopentrep/Travel_pb2.py", line 4, in <module>
    from google.protobuf import descriptor as _descriptor
ImportError: No module named google.protobuf



Very basic runtime testing _without_ the -data subpackage being installed causes the programs to abort:

  $ opentrep-dbmgr 
  POR file-path is: /usr/share/opentrep/data/por/ori_por_public.csv
  Xapian database filepath is: /tmp/opentrep/traveldb
  Log filename is: opentrep-dbmgr.log
  Creating the SQLite3 database may take a few minutes on some architectures (and a few seconds on fastest ones)...
  terminate called after throwing an instance of 'OPENTREP::SQLDatabaseException'
  what():  Error when trying to retrieve 0-th row from the SQLite3 database:Cannot establish connection to the database. unable to open database file
  Aborted (core dumped)

Only after installing the -data package, running opentrep-dbmgr opentrep-indexer opentrep-searcher succeeds.

Comment 23 Denis Arnaud 2014-04-13 21:50:41 UTC
Spec URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep.spec
SRPM URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep-0.6.1-1.fc20.src.rpm

Successful Koji build (for the x86_64 architecture):
http://koji.fedoraproject.org/koji/taskinfo?taskID=6733700
Successful Koji build (for all the architectures):
http://koji.fedoraproject.org/koji/taskinfo?taskID=6733709

Now everything appears to work as expected*.

There is still the warning about the Python .so library in a non -devel sub-package (at it is in the -python sub-package):
opentrep-python.x86_64: W: devel-file-in-non-devel-package /usr/lib64/python2.7/site-packages/libpyopentrep/libpyopentrep.so

If you have any idea as how to remove that warning, it is welcome!

Comment 24 Denis Arnaud 2014-04-13 22:03:47 UTC
(In reply to Michael Schwendt from comment #22)

Many thanks, Michael!


Details:
> pkgconfig deps are automatic for Fedora and EL6

Removed for EL <= 5


Data sub-package:
> %package        data
> Summary:        Referential data for the %{name} library
> Group:          Applications/Databases


> Also just for completeness, "fedora-review -b 866265" reports 
> 
>   BSL (v1.0)
>   ----------
>   opentrep-0.6.0/test/parsers/full_calculator.cpp
>   opentrep-0.6.0/test/parsers/parameter_parser.cpp

Those two source code files have been removed.


> > %package        data
> 
> It's possible to put an own "License: CC-BY-SA" tag into this subpackage.

Done


> /usr/bin/opentrep-config will still conflict, because of the line
> 
>   #libdir=@libdir@

Fixed


A -python sub-package has been added.

> The Provides for the Python module libpyopentrep.so.0.6 are [potentially]
> problematic: 
> https://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering
> 
>   $ rpm -qp --provides opentrep-0.6.0-1.fc20.x86_64.rpm 
> * libpyopentrep.so.0.6()(64bit)

Fixed thanks to:
%global _privatelibs libpy%{name}[.]so.*
%global __provides_exclude ^(%{_privatelibs})$
%global __requires_exclude ^(%{_privatelibs})$


> > Note that the Python script has got a manual page,
> > but can only be executed from the Python site-packages directory.
> > I do not know how to solve that issue.
> 
> It could be moved into /usr/bin after deleting the sys.path.append lines
> which are commented out anyway. Its Python module is stored in Python search
> path.

If the Xapian indexation has been performed:
$ opentrep-indexer # It may take a few minutes...
$ # To check it:
$ opentrep-searcher -q txl sfo

Then, the following now works:
$ /usr/lib64/python2.7/site-packages/libpyopentrep/pyopentrep


> $ /usr/lib64/python2.7/site-packages/libpyopentrep/pyopentrep
> Traceback (most recent call last):
>   File "/usr/lib64/python2.7/site-packages/libpyopentrep/pyopentrep", line
> 7, in <module>
>     import Travel_pb2
>   File "/usr/lib64/python2.7/site-packages/libpyopentrep/Travel_pb2.py",
> line 4, in <module>
>     from google.protobuf import descriptor as _descriptor
> ImportError: No module named google.protobuf

A Requires: has been added for the -python sub-package.


> Very basic runtime testing _without_ the -data subpackage being installed
> causes the programs to abort:
> 
>   $ opentrep-dbmgr 
>   POR file-path is: /usr/share/opentrep/data/por/ori_por_public.csv
>   Xapian database filepath is: /tmp/opentrep/traveldb
>   Log filename is: opentrep-dbmgr.log
>   Creating the SQLite3 database may take a few minutes on some architectures
> (and a few seconds on fastest ones)...
>   terminate called after throwing an instance of
> 'OPENTREP::SQLDatabaseException'
>   what():  Error when trying to retrieve 0-th row from the SQLite3
> database:Cannot establish connection to the database. unable to open
> database file
>   Aborted (core dumped)

1. A Requires: dependency has been added onto the -data sub-package.
2. For a typical sequence of commands for opentrep-dbmgr:
$ opentrep-dbmgr
opentrep> tutorial

--------------
A few explanations:
* Referential data file is provided as a CSV file (/usr/share/opentrep/data/por/ori_por_public.csv). That data file must first be Xapian-indexed by the 'opentrep-indexer' batch program, which stores the Xapian index database/structure into /tmp/opentrep/xapian_traveldb/. That program does not use, by default, any relational database.
* The 'opentrep-searcher -q <my_query>' batch program analyses the given query against the Xapian index, and displays the results. That program does not use, by default, any relational database. The Python wrapper (/usr/lib64/python2.7/site-packages/libpyopentrep/pyopentrep) is an alternative for the same process. For instance:
$ /usr/lib64/python2.7/site-packages/libpyopentrep/pyopentrep -f F txl sfo
gives a result equivalent to:
$ opentrep-searcher -q txl sfo
* The 'opentrep-dbmgr' interactive program allows to create, update and query a relational database (for now, only SQLite3 and MySQL/MariaDB). When we are only interested by (IATA, ICAO, FAA) codes, then 'opentrep-dbmgr' is enough (there is no need for the Xapian index).

Comment 25 Michael Schwendt 2014-05-16 10:30:45 UTC
> opentrep-python.x86_64: W: devel-file-in-non-devel-package
> /usr/lib64/python2.7/site-packages/libpyopentrep/libpyopentrep.so
>
> If you have any idea as how to remove that warning, it is welcome!

It's a limitation of the fedora-review tool, which apparently doesn't recognize Python's module path as a valid location for .so files.

https://fedorahosted.org/FedoraReview/ - filing an RFE might lead to something.


> $ rpmls opentrep-data-0.6.1-1.fc20.noarch.rpm 
> -rw-r--r--  /usr/share/opentrep/data/por/README.md
> -rw-r--r--  /usr/share/opentrep/data/por/ori_por_public.csv

It doesn't require the base package yet for proper directory ownership:

  $ rpm -qpR opentrep-data-0.6.1-1.fc20.noarch.rpm |grep -v ^rpm
  $

If it shall be possible for the -data subpackage to be installed independently, you may include the directories in it, too.

Currently, the dependency is backwards, i.e. the base package requires the -data package.


> $ grep lib64 rpms-unpacked/opentrep-devel-0.6.1-1.fc20.x86_64.rpm/usr/bin/opentrep-config 
> #libdir=/usr/lib64

It would still cause a conflict, because of that line.


Other conflicts found in /usr/share/opentrep/CMake files: /usr/share is for arch-independent data, but several of the CMake files refer to /usr/lib64.


A wrong Python path has been spotted in them, too:

$ grep python/open *
opentrep-config.cmake:set (OPENTREP_LIBRARY_DIRS "/usr/lib64:/usr/lib64/python/opentrep")
opentrep-library-depends-debug.cmake:  IMPORTED_LOCATION_DEBUG "/usr/lib64/python/opentrep/libpyopentrep.so.0.6.1"
opentrep-library-depends-debug.cmake:list(APPEND _IMPORT_CHECK_FILES_FOR_pyopentreplib "/usr/lib64/python/opentrep/libpyopentrep.so.0.6.1" )


> Then, the following now works:
> $ /usr/lib64/python2.7/site-packages/libpyopentrep/pyopentrep

/usr/share/man/man1/pyopentrep.1.gz

$ pyopentrep
bash: pyopentrep: command not found...

I still think it's a bad idea to include the manual page in section 1 without making available the pyopentrep command in $PATH. Since the -python subpkg is arch-specific and not multilib'ed  it would be possible to symlink /usr/bin/pyopentrep to the script in Python module path.  (cf. comment 22)


> $ pkg-config --variable=docdir opentrep
> /usr/share/doc/opentrep-0.6.1

Invalid for Fedora >= 20 unversioned docdirs.

The opentrep-config script also contains this path, albeit unused in the script.


> /usr/share/aclocal/opentrep.m4

One place that mentions Boost as requirement. Some of the header files want Boost headers, too, but the -devel package does not depend on any Boost -devel packages yet. Should the pkgconfig file depend on Boost, too?


All are issues that sometimes are (re)introduced into packages also some time after review - any suggestions on how to proceed?

Comment 26 Michael Schwendt 2014-05-16 10:38:50 UTC
> It's a limitation of the fedora-review tool,

rpmlint precisely

Comment 27 Denis Arnaud 2014-05-16 22:10:50 UTC
Many thanks, Michael, for your thorough review, as usual!

(In reply to Michael Schwendt from comment #25)
> 
> All are issues that sometimes are (re)introduced into packages also some
> time after review - any suggestions on how to proceed?

Well, it may be because I also work on the (upstream) source code in parallel. However, on the Python related issues, it is because I still do not fully understand how to package the binaries and libraries. Review after review, I hopefully progress (at least, it is my impression): you are a very good trainer :)
So, if you are still patient enough to support me, I will eventually get it!

So, I now need to review all this and fix those issues.

Kr

D

Comment 28 Michael Schwendt 2014-05-18 14:09:12 UTC
Spotting issues or oddities often is the easy part during review. ;) Fixing them may be more difficult.

Comment 29 Denis Arnaud 2015-04-19 15:43:13 UTC
Upstream update:
Spec URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep.spec
SRPM URL: <srpm info here>
Description: http://denisarnaud.fedorapeople.org/opentrep/opentrep-0.6.5-1.fc20.src.rpm
-------
Successful builds:
http://koji.fedoraproject.org/koji/taskinfo?taskID=9514193
-------

The (Python-related) non blocking issues raised by Michael still need some attention. The package could be approved as is, though.

Comment 30 Denis Arnaud 2019-01-16 18:37:58 UTC
Upstream update:
Spec URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep.spec (and https://github.com/fedorapackaging/fedorareviews/blob/trunk/reviews/opentrep/opentrep_866265/opentrep.spec)
SRPM URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep-0.07.1-1.fc30.src.rpm
-------
Successful builds:
http://koji.fedoraproject.org/koji/taskinfo?taskID=32069647
-------

The Python part is now aligned with the latest packaging guidelines, as for instance seen on https://bugzilla.redhat.com/show_bug.cgi?id=1630837 . The Python scripts work out of the box, including with pipenv.

Test:
1. sudo dnf -y install https://kojipkgs.fedoraproject.org//work/tasks/9648/32069648/opentrep-0.07.1-1.fc30.x86_64.rpm https://kojipkgs.fedoraproject.org//work/tasks/9648/32069648/opentrep-data-0.07.1-1.fc30.noarch.rpm https://kojipkgs.fedoraproject.org//work/tasks/9648/32069648/opentrep-devel-0.07.1-1.fc30.x86_64.rpm https://kojipkgs.fedoraproject.org//work/tasks/9648/32069648/python3-opentrep-0.07.1-1.fc30.x86_64.rpm
2. pip3 install --user simplejson
3. opentrep-indexer
4. du -hs /tmp/opentrep/
92M     /tmp/opentrep/
5. opentrep-searcher # No Python
6. pyopentrep # With the Python wrapper
ORI-maintained list of POR (points of reference): '/usr/share/opentrep/data/por/optd_por_public.csv'
Xapian-based travel database/index: '/tmp/opentrep/xapian_traveldb0'
SQLite database: '/tmp/opentrep/sqlite_travel.db'
searchString: sna francicso rio de janero lso angles reykyavki
Compact format => recognised place (city/airport) codes:
SFO RIO LAX REK
------------------

So, it works!

It could eventually been approved, then :)

Comment 31 Robert-André Mauchin 🐧 2019-02-15 22:53:08 UTC
 - Please don't glob the major soname version to avoid unintentional soname bump:

%{_libdir}/lib%{name}.so.0*

 - The license file COPYING must be installed with %license, not %doc:

%files
%doc AUTHORS ChangeLog NEWS README.md
%license COPYING




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

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


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 COPYING is not marked as %license
  See:
  http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text


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

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Development (unversioned) .so files in -devel subpackage, if present.
     Note: Unversioned so-files in private %_libdir subdirectory (see
     attachment). Verify they are not in ld path.
[x]: If your application is a C or C++ application you must list a
     BuildRequires against gcc, gcc-c++ or clang.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig not called in %post and %postun for Fedora 28 and later.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

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: "FSF All Permissive License", "*No copyright* GNU Lesser
     General Public License", "Unknown or generated", "GNU Free
     Documentation License", "GNU Lesser General Public License (v2.1 or
     later)", "*No copyright* Public domain", "GNU Free Documentation
     License (v1.2 or later)", "*No copyright* GNU General Public License",
     "NTP License", "*No copyright* BSD 3-clause "New" or "Revised"
     License". 447 files have unknown license. Detailed output of
     licensecheck in /home/bob/packaging/review/opentrep/review-
     opentrep/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
[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]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 71680 bytes in 5 files.
[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]: 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 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]: 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

Python:
[x]: Python eggs must not download any dependencies during the build
     process.
[x]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
[x]: Package contains BR: python2-devel or python3-devel
[x]: Packages MUST NOT have dependencies (either build-time or runtime) on
     packages named with the unversioned python- prefix unless no properly
     versioned package exists. Dependencies on Python packages instead MUST
     use names beginning with python2- or python3- as appropriate.
[x]: Python packages must not contain %{pythonX_site(lib|arch)}/* in %files
[x]: Binary eggs must be removed in %prep

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

Generic:
[-]: 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
     opentrep-data , opentrep-doc , opentrep-debuginfo , opentrep-
     debugsource
[?]: Package functions as described.
[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.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %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: opentrep-0.07.1-1.fc30.x86_64.rpm
          opentrep-devel-0.07.1-1.fc30.x86_64.rpm
          opentrep-data-0.07.1-1.fc30.noarch.rpm
          opentrep-doc-0.07.1-1.fc30.noarch.rpm
          python3-opentrep-0.07.1-1.fc30.x86_64.rpm
          opentrep-debuginfo-0.07.1-1.fc30.x86_64.rpm
          opentrep-debugsource-0.07.1-1.fc30.x86_64.rpm
          opentrep-0.07.1-1.fc30.src.rpm
python3-opentrep.x86_64: W: dangerous-command-in-%post ln
python3-opentrep.x86_64: W: dangerous-command-in-%postun rm
8 packages and 0 specfiles checked; 0 errors, 2 warnings.

Comment 32 Denis Arnaud 2019-02-18 00:29:35 UTC
(In reply to Robert-André Mauchin from comment #31)
>  - Please don't glob the major soname version to avoid unintentional soname bump:
> 
> %{_libdir}/lib%{name}.so.0*
> 
>  - The license file COPYING must be installed with %license, not %doc:
> 
> %files
> %doc AUTHORS ChangeLog NEWS README.md
> %license COPYING

Thanks Robert-André!

Your feedback has been integrated into the new version (0.07.1-2):
Spec URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep.spec
SRPM URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep-0.07.1-2.fc30.src.rpm

Comment 33 Denis Arnaud 2019-02-18 00:30:11 UTC
Successful build: https://koji.fedoraproject.org/koji/taskinfo?taskID=32875723

Comment 34 Robert-André Mauchin 🐧 2019-02-18 17:11:39 UTC
LGTM, package approved.

Comment 35 Gwyn Ciesla 2019-02-20 21:28:53 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/opentrep

Comment 36 Fedora Update System 2019-02-23 02:39:06 UTC
opentrep-0.07.1-2.fc29 has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2019-658253422d

Comment 37 Fedora Update System 2019-02-23 21:34:28 UTC
opentrep-0.07.1-2.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2019-574d904e91

Comment 38 Fedora Update System 2019-02-24 02:17:26 UTC
opentrep-0.07.1-2.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2019-574d904e91

Comment 39 Fedora Update System 2019-02-24 03:25:29 UTC
opentrep-0.07.1-2.fc29 has been pushed to the Fedora 29 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-658253422d

Comment 40 Fedora Update System 2019-03-04 00:51:24 UTC
opentrep-0.07.1-2.fc29 has been pushed to the Fedora 29 stable repository. If problems still persist, please make note of it in this bug report.

Comment 41 Fedora Update System 2019-03-11 20:55:20 UTC
opentrep-0.07.1-2.el7 has been pushed to the Fedora EPEL 7 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.