[Bug 478470] Review Request: mrpt - The Mobile Robot Programming Toolkit (MRPT)
bugzilla at redhat.com
bugzilla at redhat.com
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.
More information about the Fedora-package-review
mailing list