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.
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.
Thanks Volker for the review. I will have a look at it shortly and publish the amended versions of the package.
[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.
> %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.
Thanks Michael! The package is still waiting for an approval. So, do not hesitate to come forward :)
Spec URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep.spec SRPM URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep-0.5.2-1.fc18.src.rpm
Spec URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep.spec SRPM URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep-0.5.3-1.fc18.src.rpm
Spec URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep.spec SRPM URL: http://denisarnaud.fedorapeople.org/opentrep/opentrep-0.5.3-2.fc19.src.rpm Successful build on Koji for Rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=5804997
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.
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).
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!
ping.
(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...
(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.
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.
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
(In reply to Michael Schwendt from comment #16) Many thanks Michael, that is very helpful. I will have a look whenever I can.
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.
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!
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.
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.
> %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.
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!
(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).
> 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?
> It's a limitation of the fedora-review tool, rpmlint precisely
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
Spotting issues or oddities often is the easy part during review. ;) Fixing them may be more difficult.
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.
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 :)
- 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.
(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
Successful build: https://koji.fedoraproject.org/koji/taskinfo?taskID=32875723
LGTM, package approved.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/opentrep
opentrep-0.07.1-2.fc29 has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2019-658253422d
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
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
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
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.
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.