https://bugzilla.redhat.com/show_bug.cgi?id=829097
Bug ID: 829097 QA Contact: extras-qa@fedoraproject.org Severity: medium Version: rawhide Priority: medium CC: notting@redhat.com, package-review@lists.fedoraproject.org Assignee: nobody@fedoraproject.org Summary: Review Request: sicktoolbox - The SICK LIDAR Toolbox Regression: --- Story Points: --- Classification: Fedora OS: Linux Reporter: richmattes@gmail.com Type: --- Documentation: --- Hardware: All Mount Type: --- Status: NEW Component: Package Review Product: Fedora
Spec URL: http://rmattes.fedorapeople.org/RPMS/sicktoolbox/sicktoolbox.spec SRPM URL: http://rmattes.fedorapeople.org/RPMS/sicktoolbox/sicktoolbox-1.0.1-1.fc17.sr...
Description: The Sick LIDAR Toolbox is an open-source software package released under a BSD Open-Source License that provides stable and easy-to-use C++ drivers for Sick LMS 2xx and Sick LD laser range finders.
Fedora Account System Username: rmattes
$ rpmlint sicktoolbox.spec ../RPMS/x86_64/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 3 packages and 1 specfiles checked; 0 errors, 3 warnings.
Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4130527
https://bugzilla.redhat.com/show_bug.cgi?id=829097
Volker Fröhlich volker27@gmx.at changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |volker27@gmx.at
--- Comment #1 from Volker Fröhlich volker27@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.
https://bugzilla.redhat.com/show_bug.cgi?id=829097
--- Comment #2 from Rich Mattes richmattes@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.sr...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=829097
Rich Mattes richmattes@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
https://bugzilla.redhat.com/show_bug.cgi?id=829097
Ralf Corsepius rc040203@freenet.de changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |rc040203@freenet.de Assignee|nobody@fedoraproject.org |rc040203@freenet.de Flags| |fedora-review?
--- Comment #3 from Ralf Corsepius rc040203@freenet.de --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=829097
--- Comment #4 from Rich Mattes richmattes@gmail.com --- 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.sr...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=829097
Ralf Corsepius rc040203@freenet.de changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
--- Comment #5 from Ralf Corsepius rc040203@freenet.de --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=829097
--- Comment #6 from Rich Mattes richmattes@gmail.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=829097
Rich Mattes richmattes@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-cvs?
--- Comment #7 from Rich Mattes richmattes@gmail.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=829097
Jon Ciesla limburgher@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-cvs? |fedora-cvs+
https://bugzilla.redhat.com/show_bug.cgi?id=829097
--- Comment #8 from Jon Ciesla limburgher@gmail.com --- Git done (by process-git-requests).
https://bugzilla.redhat.com/show_bug.cgi?id=829097
--- Comment #9 from Fedora Update System updates@fedoraproject.org --- sicktoolbox-1.0.1-4.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2015-16137
https://bugzilla.redhat.com/show_bug.cgi?id=829097
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |MODIFIED
https://bugzilla.redhat.com/show_bug.cgi?id=829097
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
--- Comment #10 from Fedora Update System updates@fedoraproject.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=829097
--- Comment #11 from Fedora Update System updates@fedoraproject.org --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=829097
Fedora Update System updates@fedoraproject.org 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
package-review@lists.fedoraproject.org