Bug 866265 - Review Request: opentrep - C++ API for parsing travel-focused requests
Review Request: opentrep - C++ API for parsing travel-focused requests
Status: NEW
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-14 19:42 EDT by Denis Arnaud
Modified: 2015-04-19 11:43 EDT (History)
6 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:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Denis Arnaud 2012-10-14 19:42:07 EDT
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 17:17:46 EDT
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 19:10:30 EDT
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 17:11:48 EDT
[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 15:28:35 EST
> %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 18:26:24 EST
Thanks Michael!
The package is still waiting for an approval. So, do not hesitate to come forward :)
Comment 9 Michael Schwendt 2013-08-12 06:35:29 EDT
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 08:16:11 EDT
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-12 21:10:42 EDT
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 09:19:52 EST
ping.
Comment 13 Denis Arnaud 2013-11-03 12:57:08 EST
(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 07:07:17 EST
(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 07:38:30 EST
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 08:37:53 EST
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 15:53:01 EST
(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 17:26:54 EST
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-02 21:11:15 EST
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 12:27:35 EST
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 13:58:14 EST
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 07:22:53 EST
> %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 17:50:41 EDT
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 18:03:47 EDT
(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 06:30:45 EDT
> 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 06:38:50 EDT
> It's a limitation of the fedora-review tool,

rpmlint precisely
Comment 27 Denis Arnaud 2014-05-16 18:10:50 EDT
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 10:09:12 EDT
Spotting issues or oddities often is the easy part during review. ;) Fixing them may be more difficult.
Comment 29 Denis Arnaud 2015-04-19 11:43:13 EDT
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.

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