Bug 478470 - Review Request: mrpt - The Mobile Robot Programming Toolkit (MRPT)
Review Request: mrpt - The Mobile Robot Programming Toolkit (MRPT)
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-12-30 13:27 EST by Jose Luis
Modified: 2009-08-12 20:05 EDT (History)
3 users (show)

See Also:
Fixed In Version: 0.6.5-0.3.20090213svn807.fc10
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-02-24 15:44:16 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
rpmlint for binary rpms (6.32 KB, text/plain)
2009-01-10 12:57 EST, Mamoru TASAKA
no flags Details

  None (edit)
Description Jose Luis 2008-12-30 13:27:09 EST
Spec URL: http://babel.isa.uma.es/mrpt/src-repo/rpm/mrpt.spec
SRPM URL: http://babel.isa.uma.es/mrpt/src-repo/rpm/mrpt-0.6.5svn714-1.src.rpm
Description: 
The Mobile Robot Programming Toolkit (MRPT) is an extensive, cross-platform,
and open source C++ library aimed to help robotics researchers to design and
implement algorithms in the fields of Simultaneous Localization and Mapping 
(SLAM), computer vision, and motion planning (obstacle avoidance).

The libraries include classes for easily managing 3D(6D) geometry, 
probability density functions (pdfs) over many predefined variables (points 
and poses, landmarks, maps), Bayesian inference (Kalman filters, particle 
filters), image processing, path planning and obstacle avoidance, 3D 
visualization of all kind of maps (points, occupancy grids, landmarks,...), 
etc.
Gathering, manipulating and inspecting very large robotic datasets (Rawlogs)
efficiently is another goal of MRPT, supported by several classes and 
applications.

The MRPT is free software and is released under the GPL.
Comment 1 Jose Luis 2008-12-30 13:29:07 EST
This is my first package, so I'm seeking a sponsor. Any feedback will be appreciated!

Thanks.
Comment 2 Mamoru TASAKA 2009-01-02 12:45:43 EST
Some notes
- First of all, your package does not build:
  http://koji.fedoraproject.org/koji/taskinfo?taskID=1029717
  So I just check your spec file

- After some long discussion, many Fedora contributors say that
  including the package name itself in its Summary is redundant.

- By the way, is it really needed that the most part of %description
  is repeated on every subpackage?

- For source tarball:
  I cannot see rev.714 tarball on the URL. Also your versioning
  of this srpm is not proper for Fedora. Please refer to
  https://fedoraproject.org/wiki/Packaging/SourceURL#Using_Revision_Control
  https://fedoraproject.org/wiki/Packaging/NamingGuidelines#SnapshotPackages

  ! Also please consider to use %{?dist} tag:
    https://fedoraproject.org/wiki/Packaging/DistTag

- Please remove "Packager" item. This is automatically defined when
  rebuilding this srpm on Fedora site.

- "%package -n mrpt-ann" can simply be replaced by "%package ann" (same
  for other subpackages)

- Usually the dependencies between packages created from the same srpm
  must be EVR (Epoch:Version:Release) specific, not just name specific
  (i.e. usually should have "Requires: %{name}-mrpt = %{version}-%{release}",
   for example).

- Packages containing pkgconfig .pc file must have "Requires: pkgconfig"
  (ref:
   https://fedoraproject.org/wiki/Packaging/ReviewGuidelines )

- Currently I am not sure if koji supports noarch subpackages build
  when main package is arch dependent, however for now I doubt this
  (and other packages don't this so).
  So please remove "BuildArch: noarch" for -doc subpackage.

- Make build.log more verbose (to check if compiler options are correctly
  honored, for example). "make VERBOSE=1" seems to work for this package
  (ref:
   https://fedoraproject.org/wiki/Packaging/cmake )

- Pleae call "make documentation_html" at %build stage
  (By the way, why is it needed to call "make %?_smp_mflags" at %install ?)

- Please take care of directory ownership issue.
  https://fedoraproject.org/wiki/Packaging/Guidelines#File_and_Directory_Ownership
  https://fedoraproject.org/wiki/Packaging/UnownedDirectories
  - This package must not own %{_datadir}/applications/,
    %{_libdir}/pkgconfig or so
  - Instead %_datadir/%name is not owned by any packages

- %post -n mrpt-apps -p '/usr/bin/update-mime-database /usr/share/mime'
  This cannot be done (you can simply think that this method can be
  used only for calling /sbin/ldconfig with no other additional
  scripts)

- For %changelog format, please follow
  https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs
Comment 3 Jose Luis 2009-01-03 12:20:53 EST
Mamoru, thanks a lot for all the notes!
I'll write here again when I can process all the information and fix everything.
Comment 4 Jose Luis 2009-01-05 13:41:36 EST
Well, I've fixed all the issues commented by Mamoru Tasaka, plus:
- Use of the release version 0.6.4 instead of the SVN snapshot, since there are no important changes and that seems to complicate a lot the naming of packages.
- Added missing build dependencies.
- Added generation of man pages, forgotten in the previous version. By the way, is there any specific command within %files for installing man-pages? I've added them manually with %{_datadir}/man/man1/*


These are the NEW files:

Spec URL: http://babel.isa.uma.es/mrpt/src-repo/rpm/mrpt.spec
SRPM URL: http://babel.isa.uma.es/mrpt/src-repo/rpm/mrpt-0.6.4-1.fc10.src.rpm


About the repetition of %description, I've shortened all the texts, leaving only one short part common to all the sub-packages.

I think everything is now much more aligned to Fedora policies. Let me know if there are new errors.

Thanks.
Comment 5 Mamoru TASAKA 2009-01-06 11:46:35 EST
Well,

- On x86_64 test fails:
  http://koji.fedoraproject.org/koji/taskinfo?taskID=1035263

- On ppc it doesn't build
  http://koji.fedoraproject.org/koji/taskinfo?taskID=1035304
Comment 6 Jose Luis 2009-01-07 20:02:52 EST
It's wierd... The error on ppc seems to come from a compiler flag only present on one of the libraries, and not the others. I think I've fixed it, please re-download the files:

Spec URL: http://babel.isa.uma.es/mrpt/src-repo/rpm/mrpt.spec
SRPM URL: http://babel.isa.uma.es/mrpt/src-repo/rpm/mrpt-0.6.4-1.fc10.src.rpm

About %check, I've enabled a more verbose output to figure out what's going on there... the same code has compiled & tested OK on all platforms in Debian & Ubuntu autobuild machines, so I have no idea what's wrong here... If the more verbose output does not give any clue, I think the %check section could be removed as the last choice.

Regards,
JL
Comment 7 Mamoru TASAKA 2009-01-07 22:21:44 EST
Before I check your srpm:

Please change the release number every time you modify your
srpm/spec to avoid confusion:
https://fedoraproject.org/wiki/Packaging/FrequentlyMadeMistakes
Comment 8 Jose Luis 2009-01-08 15:04:33 EST
All right, done.
 
Here are the new files then:
Spec URL: http://babel.isa.uma.es/mrpt/src-repo/rpm/mrpt.spec
SRPM URL: http://babel.isa.uma.es/mrpt/src-repo/rpm/mrpt-0.6.4-2.fc10.src.rpm
Comment 9 Mamoru TASAKA 2009-01-10 12:57:38 EST
Created attachment 328632 [details]
rpmlint for binary rpms

Well, for 0.6.4-2:

** Description etc
* License
  - License tag should be "GPLv3+".

* Source
  - Source tarball in your srpm differs from what I could
    download from the Source URL in your spec:
------------------------------------------------
9773759 2009-01-09 00:43 mrpt-0.6.4.tar.gz
9773812 2009-01-09 00:51 mrpt-0.6.4-2.fc10.src/mrpt-0.6.4.tar.gz
------------------------------------------------

* 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.

** %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.

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

    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.

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

    Possible solution:
    - For 2,3: at %prep:
-------------------------------------------
%prep
%setup -q
sed -i.misc \
	-e 's|-O3|-O2|' \
	-e 's|-mtune=native||' \
	-e '/DESTINATION/s|PREFIX}lib|PREFIX}lib\${LIB_SUFFIX}|' \
	$(find . -name CMakeLists.txt)
-------------------------------------------

    - For 1: at %check:
-------------------------------------------
%check
export LD_LIBRARY_PATH=$(pwd)/lib
make test VERBOSE=1 ARGS="-VV"
-------------------------------------------

    - For 4: Check %files list

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

* desktop files
  - desktop-file-install or so must be executed against
    desktop files to be installed.
    https://fedoraproject.org/wiki/Packaging/Guidelines#desktop-file-install_usage

* 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

* 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)
  - %{_datadir}/doc/mrpt-doc/ is not owned by any packages.

* 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.

  - 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
--------------------------------------------
    Please check if these explicit linkage lists are _really_
    needed. I guess almost all of these can be removed
    if the installed libraries are already properly linked.

    Also, note that each "-lfoo" entry adds each corresponding
    package dependency to mrpt-devel package. For example
    "-lglut" means -devel package should have 
    "Requires: freeglut-devel", while I don't think this is needed.

* 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).

* 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.

    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.

* 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
Comment 10 Mamoru TASAKA 2009-01-17 08:32:57 EST
ping?
Comment 11 Jose Luis 2009-01-17 15:02:47 EST
Sorry for the delay... Yes, I read everything and have some things already solved, other still pending.

I wanted to ask you about some small doubts:

1) Until now, I'm building rpms/srpms by invoking: "rpmbuild -ta TARBALL.tar.gz", where the .spec file is inside the tarball. I guess this is not the best practice, since any change in the spec file requires an update of the whole tarball. I've read the rpmbuild manpage but not found a command such as "rpmbuild -ta TARBALL.tar.gz <SOME-FLAG> mrpt.spec" which allows me to provide the spec file separately. Any ideas about this?

2) "Package-splitting: First of all, please explain why you want to split each
    library into different subpackges." I understand here the splitting into "mrpt-XXX" library packages, right? My intention is to separate dependencies, so an application (in a future package) might require only a subset of the libs. Do you want me to explicitly explain this in comments in the spec file?

Again, thanks a lot for all the time you're expending on the review. I think I'll upload a new revised version by the next week.
Comment 12 Jose Luis 2009-01-17 15:14:06 EST
I've solved my previous point (1) about rpmbuild and tarballs.
Comment 13 Mamoru TASAKA 2009-01-18 09:25:07 EST
(In reply to comment #11)
> Sorry for the delay... Yes, I read everything and have some things already
> solved, other still pending.
> 
> I wanted to ask you about some small doubts:
> 
> 1) Until now, I'm building rpms/srpms by invoking: "rpmbuild -ta
> TARBALL.tar.gz", where the .spec file is inside the tarball. I guess this is
> not the best practice, since any change in the spec file requires an update of
> the whole tarball. I've read the rpmbuild manpage but not found a command such
> as "rpmbuild -ta TARBALL.tar.gz <SOME-FLAG> mrpt.spec" which allows me to
> provide the spec file separately. Any ideas about this?

- Please use $ rpmbuild -ba foo.spec .

> 2) "Package-splitting: First of all, please explain why you want to split each
>     library into different subpackges." I understand here the splitting into
> "mrpt-XXX" library packages, right? 

- Yes.

> My intention is to separate dependencies,
> so an application (in a future package) might require only a subset of the
> libs. Do you want me to explicitly explain this in comments in the spec file?

- Please.
Comment 14 Jose Luis 2009-01-18 14:41:27 EST
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.
Comment 15 Mamoru TASAKA 2009-01-19 13:30:12 EST
Well, I have not checked your latest srpm yet (currently
I just verified that your latest srpm builds), however:

-------------------------------------------------------------
NOTE: Before being sponsored:

This package will may be accepted with another few work
(in fact I have not checked your latest srpm yet...)
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
Comment 16 Jose Luis 2009-01-19 19:05:41 EST
Ok, copy that.
Comment 17 Mamoru TASAKA 2009-01-21 10:43:44 EST
For 0.6.5-0.1:

* Tarball
  - For svn repo based tarball, please follow
    https://fedoraproject.org/wiki/Packaging/SourceURL#Using_Revision_Control

* BuildRequires
  - Some reviewers may say that "BuildRequires: tex(latex) tex(dvips)" is
    preferred.

(In reply to comment #14)
> 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?? 
    Including revision number in tarball name is highly recommeded.

* Dependency for -devel subpackage
  - Requires: opencv is not needed.
  - For this package "Requires: opencv-devel" does not seem to be needed,
    either.

* Compiler flags
  - On some parts Fedora specific compiler flags are not honored.
    For example:
--------------------------------------------------------------------------
   179  /usr/bin/cmake -E cmake_progress_report /builddir/build/BUILD/mrpt-0.6.5/CMakeFiles 
   180  [  1%] 
   181  Building CXX object otherlibs/aria-2.5.1/src/CMakeFiles/mrpt-aria.dir/ArAction.o
   182  cd /builddir/build/BUILD/mrpt-0.6.5/otherlibs/aria-2.5.1/src && /usr/bin/c++   -D_FILE_OFFSET_BITS=64 -D_LARGE_FILES -D__WXGTK__ -Dmrpt_aria_EXPORTS -O2 -fPIC -D_REENTRANT -fno-exceptions -pthread -fPIC -I/usr/include/opencv -isystem /usr/lib/wx/include/gtk2-unicode-release-2.8 -isystem /usr/include/wx-2.8 -I/builddir/build/BUILD/mrpt-0.6.5/. -I/builddir/build/BUILD/mrpt-0.6.5/include/mrpt-config/unix -I/builddir/build/BUILD/mrpt-0.6.5/include -I/builddir/build/BUILD/mrpt-0.6.5/include/mrpt/otherlibs -I/builddir/build/BUILD/mrpt-0.6.5/include/mrpt/otherlibs/aria   -o CMakeFiles/mrpt-aria.dir/ArAction.o -c /builddir/build/BUILD/mrpt-0.6.5/otherlibs/aria-2.5.1/src/ArAction.cpp
--------------------------------------------------------------------------
    (I usually check this by seeking for the word "FORTIFY")

* Directory ownership issue
  - -doc subpackage should own %{_datadir}/doc/mrpt-doc/ directory.


Also I will wait for your another review request or your pre-review
of other person's review request.
Comment 18 Jose Luis 2009-01-23 11:32:05 EST
Thanks Mamoru!  Give me a few days for the changes. And about the other review, I'll probably pick something from: 
http://fedoraproject.org/wiki/PackageMaintainers/WishList


Anyway: Is there any way to see the (latest) build logs at Koji? 
I've tried looking for "mrpt" without success... I guess the builds there are different from those in my Fedora. For example, there's a "-g" and a "-mcpu" making their way to CFLAGS and I cannot find where to remove it (some rpm config file?), and I wondered if they're not in the Koji builds.
Comment 19 Mamoru TASAKA 2009-01-23 14:11:29 EST
(In reply to comment #18)
> Anyway: Is there any way to see the (latest) build logs at Koji? 
> I've tried looking for "mrpt" without success... 

Here you mean that you want to see my scratch build of your latest
mrpt srpm on koji?
Currently mrpt is not imported into Fedora (as it is currently
under review here), so searching mrpt on koji returns nothing.

For my scratch build, please check:
For F-11:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1076797
For F-10:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1076808
! Note
  For my scratch build on koji done less than one week ago,
  they are preserved on:
  http://koji.fedoraproject.org/scratch/mtasaka/

> I guess the builds there are
> different from those in my Fedora. For example, there's a "-g" and a "-mcpu"
> making their way to CFLAGS and I cannot find where to remove it (some rpm
> config file?), and I wondered if they're not in the Koji builds.

For -mcpu I don't know, however for -g %optflags also contains "-g"
option and this must not be removed (to create debuginfo rpm)
Comment 20 Jose Luis 2009-01-23 17:48:58 EST
(In reply to comment #19)

> For my scratch build, please check:
> For F-11:
> http://koji.fedoraproject.org/koji/taskinfo?taskID=1076797
> For F-10:
> http://koji.fedoraproject.org/koji/taskinfo?taskID=1076808
> ! Note
>   For my scratch build on koji done less than one week ago,
>   they are preserved on:
>   http://koji.fedoraproject.org/scratch/mtasaka/
> 

That was exactly what I was looking for, thanks! Now with the build.log I understand that about "FORTIFY" :-)
Comment 21 Mamoru TASAKA 2009-02-06 10:22:45 EST
ping?
Comment 22 Mamoru TASAKA 2009-02-13 10:48:36 EST
(The submitter replied to me that update will come in a few
 days)
Comment 23 Jose Luis 2009-02-15 13:24:28 EST
My other submission, "cutecom":

https://bugzilla.redhat.com/show_bug.cgi?id=485636
Comment 24 Jose Luis 2009-02-15 13:26:31 EST
And this is the new review of MRPT:

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.20090213.fc10.src.rpm

Apart from the issues you detected, these are the additional changes:

* Sat Feb 13 2009 - Jose Luis Blanco <joseluisblancoc@gmail.com> 0.6.5-0.1.20090213
- New upstream sources.
- Individual packages created for each MRPT application.
- Removed unneeded dependencies from -devel package.
- Fixed "doc" package should own the mrpt-doc directory.
- Mime types moved to mrpt-core package.
Comment 25 Mamoru TASAKA 2009-02-16 13:07:26 EST
Well, would you explain why you want to split each binaries into
different subpackages? I don't think people using mrpt can get
much gain , and the naming of subpackages seems confusing because
from the first look some subpackages have names which don't seem
to be related to mrpt.
Comment 26 Jose Luis 2009-02-16 15:20:24 EST
(In reply to comment #25)
> Well, would you explain why you want to split each binaries into
> different subpackages? I don't think people using mrpt can get
> much gain , and the naming of subpackages seems confusing because
> from the first look some subpackages have names which don't seem
> to be related to mrpt.

Honestly, I wasn't unsure about this split. 

On the one hand, I do see a gain as many people will not use all mrpt programs, so they can install what I guess are the most useful ones. Each subpackage carries its own executable, plus a subdirectory in /share/ with sample script/configuration files, so I also see a good thing that only those files of the programs of interest are installed.

OTOH, I have to admit that the package namespace is messed up with this change, while there's not a huge gain in disk space.

If your final opinion is to leave all programs in mrpt-apps, I would see that fine. In that case, I might also join "mrpt-example-datasets" back into "mrpt-apps" as it was in the beginning.
Comment 27 Mamoru TASAKA 2009-02-17 11:44:37 EST
(In reply to comment #26)
> If your final opinion is to leave all programs in mrpt-apps, I would see that
> fine. In that case, I might also join "mrpt-example-datasets" back into
> "mrpt-apps" as it was in the beginning.

I think this idea (i.e. putting all programs in mrpt-apps) is much
better.

Some comments
- For tarball based on svn repository, I prefer to include revision
  number rather than the date you checked the source because revision
  number identifies the codes used in the srpm, however this is
  left to your choice.
Comment 29 Jose Luis 2009-02-18 04:08:15 EST
I forgot to comment something:

> - For tarball based on svn repository, I prefer to include revision
>   number rather than the date you checked the source because revision
>   number identifies the codes used in the srpm, however this is
>   left to your choice.

I've used the svn number of the main mrpt repository, which is different from the SVN URL given within the SPEC file. 
The reason is that one directory of MRPT contains "prohibited code" (patent-pending) so I created a separate SVN repository just for publishing "clean releases", and that is the one referenced in the specfile. That's why I didn't add the "svn -r NUMBER" to the comments there, but just the svn URL. 

However, this is not a problem, since this reference can be seen as a svn "tag" directory, not the trunk, and it'll not change in the future.
Comment 30 Mamoru TASAKA 2009-02-18 12:09:29 EST
For 0.6.5-0.2:

* Directory/file ownership issue
  - Now -core and -apps subpackages own some same files.
    I guess your intention is to move these files from -apps to -core
    (currently -apps Requires -core, so this is okay)

  - Related to this, scriptlets for -apps subpackage should no
    longer be needed.

I think you will fix this when you import this package
into Fedora CVS.

! I glanced at your another review request (bug 485636
  for cutecom) and seems good from a quick look.
  Unfortunately it may be that I don't have time to
  review your another review request soon, however I
  will pay attention to it.

-------------------------------------------------------------------
   This package (mrpt) is now APPROVED by mtasaka
-------------------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Install the Client Tools (Koji)".

Now I am sponsoring you.

If you want to import this package into Fedora 9/10, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

Removing NEEDSPONSOR.
Comment 31 Jose Luis 2009-02-19 13:58:11 EST
New Package CVS Request
=======================
Package Name: mrpt
Short Description: Libraries and programs for mobile robot SLAM and navigation
Owners: joseluisblancoc
Branches: F-9 F-10
Comment 32 Kevin Fenzi 2009-02-20 15:38:13 EST
Specified owner ID joseluisblancoc does not have a Fedora Account

Can you check and confirm thats your Fedora Account?
Comment 33 Jose Luis 2009-02-20 18:16:33 EST
Kevin, you're right. My account there is: jlblanco

Sorry for the inconveniences.
Comment 34 Kevin Fenzi 2009-02-22 13:58:25 EST
Thanks. 

cvs done.
Comment 35 Fedora Update System 2009-02-24 15:44:08 EST
mrpt-0.6.5-0.3.20090213svn807.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 36 Jose Luis 2009-08-11 02:53:41 EDT
Package Change Request
======================
Package Name: mrpt
New Branches: F-11
Owners: jlblanco

Thanks.
Comment 37 Kevin Fenzi 2009-08-12 20:05:54 EDT
There already is a F-11 branch of this package. 

Make sure you do a 'cvs update -d' to get new directories.

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