[Bug 478470] Review Request: mrpt - The Mobile Robot Programming Toolkit (MRPT)

Sun Jan 18 19:41:28 UTC 2009

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=478470

--- Comment #14 from Jose Luis <joseluisblancoc at gmail.com>  2009-01-18 14:41:27 EDT ---
Well, here is the new revision:

SPEC: http://babel.isa.uma.es/mrpt/src-repo/rpm/mrpt.spec
SRPM:
http://babel.isa.uma.es/mrpt/src-repo/rpm/mrpt-0.6.5-0.1.20090118svn746.fc10.src.rpm

I finally decided to use the "snapshot versions format" so I can integrate all
the fixes in upstream.
btw, if I got it right, a package, say "foo-1.2.3-0.1.20090118svn" MUST have an
associated tarball 
"foo-1.2.3.tar.gz", without any indication of the snapshot?? I'd prefer more
descriptive names containing 
the snapshot part, so please confirm me if that is the preferred naming or I
can create tarballs with the 
svn number prefix.

Next are the answers to your points:

(In reply to comment #9)
> ** Description etc
> * License
>   - License tag should be "GPLv3+".

Done.

> * BuildRequires:
>   - build.log says:
> ------------------------------------------------
>    107  -- Looking for doxygen...
>    108  -- Looking for doxygen... - found /usr/bin/doxygen
>    109  -- Looking for dot tool...
>    110  -- Looking for dot tool... - NOT found
> ------------------------------------------------
>     Perhaps "BuildRequires: graphviz" is missing.

This is not an issue. "dot" is actually not used. 
In fact the CMake command is "look for doxygen", but it internally also looks
for dot...

> ** %prep -> %install
> * Build failure
>   - Currently your srpm does not build by 4 reasons.
>     1 %check fails as
>        http://koji.fedoraproject.org/koji/taskinfo?taskID=1042528
>        This is because at "make test" this test needs
>        system-widely installed libmrpt-core.so.0.6, but on mockbuild
>        apparently this library is not yet installed system-widely.

Fixed with your "LD_LIBRARY_PATH..." solution.

>     2 The rebuilt libraries like libmrpt-core.so.0.6 are installed
>       under %buildroot/usr/lib, not %buildroot%_libdir even with
>       64 bits architecture

Fixed in upstream CMakeLists.txt's.

>     3 CMakeLists.txt adds "-mtune=native" to CPPFLAGS, which
>       is not recognized on ppc (maybe also on ppc64)
>       http://koji.fedoraproject.org/koji/taskinfo?taskID=1042483
>       Also, CMakeLists.txt adds "-O3" compilation option, however
>       Fedora default optimation level is "-O2" so this should
>       be removed.

Fixed through a cmake argument "-DCMAKE_MRPT_IS_RPM_PACKAGE=1" which internally 
disables "-mtune" and "O3".

>     4 %files lists are wrong. Some files are installed
>       under %{_datadir}/mrpt/datasets/ but no files are installed
>       under %{_datadir}/mrpt/config_files/datasets/

Solved.

> ** %files, etc
> * Package splitting
>   - First of all, please explain why you want to split each
>     library into different subpackges.

Comments added to specfile.

> * desktop files
>   - desktop-file-install or so must be executed against
>     desktop files to be installed.

Done, at %install.

> 
> * Scriptlets
>   - Some files has MimeType, so please refer to:
>     https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#desktop-database
> 
>   - This package installs XML files under %_datadir/mime/packages:
>     https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#mimeinfo

Both done at %post/postun of mrpt-apps.

> * Directory ownership
>   - The directory %{_datadir}/mrpt is owned by 2 packages. Please
>     change this so that only one package owns this directory
>     ( I guess having this directory owned by -core package is
>       an alternative solution)

The -core package now owns that directory.

>   - %{_datadir}/doc/mrpt-doc/ is not owned by any packages.

It's now of the package "mrpt-doc".

> * pkgconfig
>   - pkgconfig/libmrpt.pc.in contains:
> --------------------------------------------
>      3  libdir=${exec_prefix}/lib
> --------------------------------------------
>     This is expanded as /usr/lib, even on 64 bits architecture,
>     while this should be expanded as /usr/lib64.

Fixed using the LIB_SUFFIX variable in CMake (not tested...).

>   - Also installed libmrpt.pc contains:
> --------------------------------------------
>      9  Libs: -L${libdir}   -ldc1394 -lGL -lGLU -lglut -lftdi -lusb -l3ds -lz
> -ljpeg -lrt -pthread -lmrpt-ann /usr/lib64/libboost_program_options-mt.so
> -pthread -lwx_baseu-2.8 -lwx_gtk2u_core-2.8 -lwx_gtk2u_gl-2.8
> -lwx_gtk2u_adv-2.8 -lwx_gtk2u_aui-2.8 -lmrpt-core -lmrpt-aria
> --------------------------------------------

You're right! Most of the -lxxx were not required.

> * Header files dependency
>   - Please check if proper dependency packages are installed
>     with -devel packages to satisfy "include" macro dependencies
>     in header files.
>     - For example, gui/WxUtils.h contains
> ---------------------------------------------
>     40  #include <wx/sizer.h>
>     41  #include <wx/statbmp.h>
>     42  #include <wx/menu.h>
> ---------------------------------------------
>       This means that mrpt-devel package should have "Requires: wxGTK-devel"
>       (not wxGTK).

Ok, done.

> * Duplicate files
>   - Please check if all subpackages need "doc COPYING" or so.
>     Also "%doc foo" creates its own document directory (named
>     /usr/share/doc/mrpt-foo-%{version}) for each subpackage.

I tried removing COPYING and README, but rpmlint complains about packages
without 
documentation, so I finally left them.

>     Especially -doc subpackage has two document directories
>     (/usr/share/doc/mrpt-doc and /usr/share/doc/mrpt-doc-%{version})
>     I suggest these directories should be unified.

Done, /usr/share/doc/mrpt-doc is the only directory now.

> * rpmlint issue
>   rpmlint output attached.
>   - Please change non-UTF8 files to UTF-8 encoding
>   - Usually libraries themselves should not call exit()
>     (see $ rpmlint -I shared-lib-calls-exit)
>   - Many files has Windows-like line terminators.
>     Please fix these by "sed -i -e 's|\r||g'" or dos2unix

All fixed.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.