[Bug 829097] Review Request: sicktoolbox - The SICK LIDAR Toolbox

[bugzilla]

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



--- Comment #11 from Fedora Update System  ---
sicktoolbox-1.0.1-4.fc23 has been pushed to the Fedora 23 stable repository. If
problems still persist, please make note of it in this bug report.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 829097] Review Request: sicktoolbox - The SICK LIDAR Toolbox

[bugzilla]

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

Fedora Update System  changed:

   What|Removed |Added

 Status|ON_QA   |CLOSED
   Fixed In Version||1.0.1-4.fc23
 Resolution|--- |NEXTRELEASE
Last Closed||2015-09-24 01:11:46



-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 829097] Review Request: sicktoolbox - The SICK LIDAR Toolbox

[bugzilla]

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

Fedora Update System  changed:

   What|Removed |Added

 Status|MODIFIED|ON_QA



--- Comment #10 from Fedora Update System  ---
sicktoolbox-1.0.1-4.fc23 has been pushed to the Fedora 23 testing repository.
If problems still persist, please make note of it in this bug report.\nIf you
want to test the update, you can install it with \n su -c 'yum
--enablerepo=updates-testing update sicktoolbox'. You can provide feedback for
this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-16137

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 829097] Review Request: sicktoolbox - The SICK LIDAR Toolbox

[bugzilla]

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



--- Comment #8 from Jon Ciesla  ---
Git done (by process-git-requests).

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 829097] Review Request: sicktoolbox - The SICK LIDAR Toolbox

[bugzilla]

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

Jon Ciesla  changed:

   What|Removed |Added

  Flags|fedora-cvs? |fedora-cvs+



-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 829097] Review Request: sicktoolbox - The SICK LIDAR Toolbox

[bugzilla]

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

Fedora Update System  changed:

   What|Removed |Added

 Status|ASSIGNED|MODIFIED



-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 829097] Review Request: sicktoolbox - The SICK LIDAR Toolbox

[bugzilla]

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



--- Comment #9 from Fedora Update System  ---
sicktoolbox-1.0.1-4.fc23 has been submitted as an update to Fedora 23.
https://bodhi.fedoraproject.org/updates/FEDORA-2015-16137

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 829097] Review Request: sicktoolbox - The SICK LIDAR Toolbox

[bugzilla]

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



--- Comment #6 from Rich Mattes  ---
Thanks for the review!

1. I opted for sick_ instead of sick- because there are already underscores in
lms_config and ld_config, and it seemed more consistent to call the program
sick_lms_config instead of sick-lms_config.

2. I'll remove the line that removes .a files when I import the RPM.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 829097] Review Request: sicktoolbox - The SICK LIDAR Toolbox

[bugzilla]

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

Rich Mattes  changed:

   What|Removed |Added

  Flags||fedora-cvs?



--- Comment #7 from Rich Mattes  ---
New Package SCM Request
===
Package Name: sicktoolbox
Short Description: The SICK LIDAR Toolbox
Upstream URL: http://sicktoolbox.sourceforge.net
Owners: rmattes
Branches: f21 f22 f23 el6 epel7

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 829097] Review Request: sicktoolbox - The SICK LIDAR Toolbox

[bugzilla]

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

Ralf Corsepius  changed:

   What|Removed |Added

  Flags|fedora-review?  |fedora-review+



--- Comment #5 from Ralf Corsepius  ---
APPROVED

2 final remarks:

- I'd prefer the program to be prefixed with "sick-".
But, ... this is largely a matter of personal test ;)

- With --disable-static, there is no need for this:
rm -rf %{buildroot}%{_libdir}/*.a

Please remove it.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 829097] Review Request: sicktoolbox - The SICK LIDAR Toolbox

[bugzilla]

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



--- Comment #4 from Rich Mattes  ---
Thanks Ralf.  I am still interested in reviewing this package.  Upstream is
unresponsive, but it's still widely used by others in the robotics community.

I've updated the spec to pass the configure arguments --disable-static and
--program-prefix=sick_

You can find the updated spec and SRPM at:
Spec URL: http://rmattes.fedorapeople.org/RPMS/sicktoolbox/sicktoolbox.spec
SRPM URL:
http://rmattes.fedorapeople.org/RPMS/sicktoolbox/sicktoolbox-1.0.1-3.fc22.src.rpm

rpmlint output:
$ rpmlint ./sicktoolbox.spec ../RPMS/x86_64/sicktoolbox-*
../RPMS/noarch/sicktoolbox-*
sicktoolbox.x86_64: W: no-manual-page-for-binary sick_ld_config
sicktoolbox.x86_64: W: no-manual-page-for-binary sick_lms_config
sicktoolbox-devel.x86_64: W: only-non-binary-in-usr-lib
sicktoolbox-devel.x86_64: W: no-documentation
4 packages and 1 specfiles checked; 0 errors, 4 warnings.

I'll submit a scratch build once I'm off of airport wifi.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 829097] Review Request: sicktoolbox - The SICK LIDAR Toolbox

[bugzilla]

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

Ralf Corsepius  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED
 CC||rc040...@freenet.de
   Assignee|nob...@fedoraproject.org|rc040...@freenet.de
  Flags||fedora-review?



--- Comment #3 from Ralf Corsepius  ---
Rich, are you still interested in this package?

I yes, I am going to proceed with a formal review, otherwise I'd close this
request.

For the moment, some remarks:

- The package supports disabling building static libs.
  I'd suggest to 
  - %configure --disable-static
  - rm -rf %{buildroot}%{_libdir}/*.a

- I'd suggest to use a program-prefix to avoid the naming issues with
  ld/lms_config (e.g. %configure --program-prefix=sick-)
  However, this will only work if other packages don't have
  ld/lms_config hard-coded somewhere. Proposing upstream to change these
  tools' names would be appropriate, but I understand upstream is dead.
  Though I consider these names to be unfortunate, I don't see any actual
  conflicts between these and other packages.

Besides these, this package seems pretty clean to me.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 829097] Review Request: sicktoolbox - The SICK LIDAR Toolbox

[bugzilla]

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

Rich Mattes richmat...@gmail.com changed:

   What|Removed |Added

 Blocks||1225692




Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=1225692
[Bug 1225692] Tracker for Fedora Robotics SIG
-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 829097] Review Request: sicktoolbox - The SICK LIDAR Toolbox

[bugzilla]

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

Volker Fröhlich volke...@gmx.at changed:

   What|Removed |Added

 CC||volke...@gmx.at

--- Comment #1 from Volker Fröhlich volke...@gmx.at ---
Did you try to submit your patch?

Don't the other methods work that are described in
http://fedoraproject.org/wiki/Packaging:Guidelines#Removing_Rpath ?

rm -rf %{buildroot} is not necessary.

It must be:

%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig

I noticed the PDFs make up most of the size of the package. Consider a
separated -doc sub-package, as mentioned here:
http://fedoraproject.org/wiki/Packaging:Guidelines#Documentation

THANKS, NEWS and the examples directory could also be included.

I'd like to recommend registering the libray on http://upstream-tracker.org to
keep track of ABI breakage.

You can remove the trailing slash from the URL, if you want.

Odd findings: ld_config sounds a lot like ldconfig. The version numbers that
is part of the include-directories also surprised me.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 829097] Review Request: sicktoolbox - The SICK LIDAR Toolbox

[bugzilla]

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

--- Comment #2 from Rich Mattes richmat...@gmail.com ---
I just submitted the patch upstream, and included a comment in the spec (In
reply to comment #1)
 Did you try to submit your patch?
 
I hadn't, but I just did today.  The link to the upstream tracker is now in the
spec, as is a description of what the patch does

 Don't the other methods work that are described in
 http://fedoraproject.org/wiki/Packaging:Guidelines#Removing_Rpath ?
 
It looks like the second method concerning a local copy of libtool works.  I
got rid of chrpath and am instead using the sed snippets from the wiki.

 rm -rf %{buildroot} is not necessary.
 
Removed.

 It must be:
 
 %post -p /sbin/ldconfig
 %postun -p /sbin/ldconfig
 
Fixed.

 I noticed the PDFs make up most of the size of the package. Consider a
 separated -doc sub-package, as mentioned here:
 http://fedoraproject.org/wiki/Packaging:Guidelines#Documentation
 
They're a total of a little over a meg, which isn't too huge.  They're not
purely development docs either as some has to do with wiring the lasers and
with running the example programs.  I split off a doc subpackage

 THANKS, NEWS and the examples directory could also be included.
 
Included.

 I'd like to recommend registering the libray on http://upstream-tracker.org
 to keep track of ABI breakage.
 
That's definitely a useful site, but the sicktoolbox isn't really actively
developed anymore.  The last svn commits are from 2 years ago.

 You can remove the trailing slash from the URL, if you want.
 
 Odd findings: ld_config sounds a lot like ldconfig. The version numbers
 that is part of the include-directories also surprised me.

I guess they were following the lms_config example. The toolbox supports both
the LMS and LD lines of laser rangefinders, so i guess it's just an unfortunate
coincidence.  If it's a problem, I can rename the executables to
sick_lms_config and sick_ld_config.

The version number in the includedirs is strange, and the versions in the
library names are kind of annoying, but I don't know if it's annoying enough to
break compatibility with upstream.

New spec and SRPM can be found at:
Spec URL: http://rmattes.fedorapeople.org/RPMS/sicktoolbox/sicktoolbox.spec
SRPM URL:
http://rmattes.fedorapeople.org/RPMS/sicktoolbox/sicktoolbox-1.0.1-2.fc17.src.rpm

rpmlint:
$ rpmlint sicktoolbox.spec ../RPMS/x86_64/sicktoolbox-*
../RPMS/noarch/sicktoolbox-*
sicktoolbox.x86_64: W: no-manual-page-for-binary ld_config
sicktoolbox.x86_64: W: no-manual-page-for-binary lms_config
sicktoolbox-devel.x86_64: W: no-documentation
4 packages and 1 specfiles checked; 0 errors, 3 warnings.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review