Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=928584
Bug ID: 928584 Summary: Review Request: ros-std_msgs - Standard ROS Messages Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: richmattes@gmail.com QA Contact: extras-qa@fedoraproject.org CC: notting@redhat.com, package-review@lists.fedoraproject.org
Spec URL: http://rmattes.fedorapeople.org/rospackages/std-msgs/ros-std_msgs.spec SRPM URL: http://rmattes.fedorapeople.org/rospackages/std-msgs/ros-std_msgs-0.4.8-1.gi...
Description: Standard ROS Messages including common message types representing primitive data types and other basic message constructs, such as multi-arrays.
Fedora Account System Username: rmattes
rpmlint: $ rpmlint ros-std_msgs.spec ../../RPMS/noarch/ros-std_msgs-devel-0.4.8-1.gitef63d31.fc18.noarch.rpm ros-std_msgs.spec: W: invalid-url Source0: wg-debs-std_msgs-release-upstream-0.4.8-0-gef63d31.tar.gz ros-std_msgs-devel.noarch: W: spelling-error %description -l en_US multi -> mulch, mufti ros-std_msgs-devel.noarch: W: no-documentation ros-std_msgs-devel.noarch: E: zero-length /usr/share/std_msgs/msg/Empty.msg 1 packages and 1 specfiles checked; 1 errors, 3 warnings.
The spelling error is part of a hyphenated "multi-arrays" which is OK. I think the empty message needs to be empty to work properly, even if rpmlint throws an error.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=928584
Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |sanjay.ankur@gmail.com Assignee|nobody@fedoraproject.org |sanjay.ankur@gmail.com Alias| |ros-std_msgs Flags| |fedora-review?
--- Comment #1 from Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com --- Hi Rich,
Are the BuildRequires for this package correct?
BuildRequires: ros-catkin-devel BuildRequires: ros-genmsg BuildRequires: ros-gencpp BuildRequires: ros-genlisp BuildRequires: ros-genpy
We don't have a ros-catkin package, do we? We have a "catkin" package. Similarly, we don't have ros-genmsg, we have a python-genmsg etc.
I'll begin the review after you've updated the spec.
Thanks, Warm regards, Ankur
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=928584
Rich Mattes richmattes@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends On| |927470, 927473, 927475, | |927478
--- Comment #2 from Rich Mattes richmattes@gmail.com --- They're correct, the "catkin" package has a virtual provides for "ros-catkin" to make installation easier via yum (e.g. yum install ros-*) All of the other packages have similar provides, and I've added them as dependencies to this package (I forgot to when I submitted the review.)
https://bugzilla.redhat.com/show_bug.cgi?id=928584
Bug 928584 depends on bug 927470, which changed state.
Bug 927470 Summary: Review Request: python-genmsg - Python library for generating ROS message and service data https://bugzilla.redhat.com/show_bug.cgi?id=927470
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Resolution|--- |ERRATA
https://bugzilla.redhat.com/show_bug.cgi?id=928584
--- Comment #3 from Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com --- The other packages seem to be in testing. Rich, can you see if they can be pushed to stable so I can use mock/koji to test builds without using a local repo please? I'll review this package later today.
https://bugzilla.redhat.com/show_bug.cgi?id=928584
--- Comment #4 from Rich Mattes richmattes@gmail.com --- So I removed the ros-* virtual provides from most of the packages; I'll update this spec later tonight with the correct provides.
The gen{cpp,lisp,py} builds were only pushed a day or two ago, so they probably won't make it into stable until next week. You should be able to do rawhide mock builds now (once I fix the BuildRequires)
https://bugzilla.redhat.com/show_bug.cgi?id=928584
--- Comment #5 from Rich Mattes richmattes@gmail.com --- Updated packages
Spec URL: http://rmattes.fedorapeople.org/rospackages/std-msgs/ros-std_msgs.spec SRPM URL: http://rmattes.fedorapeople.org/rospackages/std-msgs/ros-std_msgs-0.4.11-2.2...
$ rpmlint ros-std_msgs.spec ../../RPMS/noarch/ros*0.4.11-2* ros-std_msgs.noarch: W: spelling-error %description -l en_US msgs -> mags, megs, mugs ros-std_msgs.noarch: W: spelling-error %description -l en_US multiarrays -> multiracial ros-std_msgs.noarch: W: no-documentation ros-std_msgs.noarch: E: zero-length /usr/share/std_msgs/msg/Empty.msg ros-std_msgs-devel.noarch: W: spelling-error Summary(en_US) msgs -> mags, megs, mugs ros-std_msgs-devel.noarch: W: spelling-error %description -l en_US msgs -> mags, megs, mugs ros-std_msgs-devel.noarch: W: no-documentation 2 packages and 1 specfiles checked; 1 errors, 6 warnings.
https://bugzilla.redhat.com/show_bug.cgi?id=928584
Rich Mattes richmattes@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |972346
https://bugzilla.redhat.com/show_bug.cgi?id=928584
--- Comment #6 from Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com --- [+] OK [-] NA [?] Issue
** Mandatory review guidelines: ** [+] rpmlint output: [asinha@localhost SRPMS]$ rpmlint ../SPECS/ros-std_msgs.spec ./ros-std_msgs-0.4.11-2.20130605gitde0dcf1.fc19.src.rpm /var/lib/mock/fedora-rawhide-x86_64/result/*.rpm ros-std_msgs.src: W: spelling-error %description -l en_US msgs -> mags, megs, mugs ros-std_msgs.src: W: spelling-error %description -l en_US multiarrays -> multiracial ros-std_msgs.noarch: W: spelling-error %description -l en_US msgs -> mags, megs, mugs ros-std_msgs.noarch: W: spelling-error %description -l en_US multiarrays -> multiracial ros-std_msgs.noarch: W: no-documentation ros-std_msgs.noarch: E: zero-length /usr/share/std_msgs/msg/Empty.msg ros-std_msgs.src: W: spelling-error %description -l en_US msgs -> mags, megs, mugs ros-std_msgs.src: W: spelling-error %description -l en_US multiarrays -> multiracial ros-std_msgs-devel.noarch: W: spelling-error Summary(en_US) msgs -> mags, megs, mugs ros-std_msgs-devel.noarch: W: spelling-error %description -l en_US msgs -> mags, megs, mugs ros-std_msgs-devel.noarch: W: no-documentation 4 packages and 1 specfiles checked; 1 errors, 10 warnings. [asinha@localhost SRPMS]$
[+] License is acceptable (...) [+] License field in spec is correct [+] License files included in package %docs if included in source package [+] License files installed when any subpackage combination is installed [+] Spec written in American English [+] Spec is legible [+] Sources match upstream unless altered to fix permissibility issues [asinha@localhost SRPMS]$ review-md5check.sh ../SPECS/ros-std_msgs.spec Getting https://github.com/ros/std_msgs/archive/de0dcf16baaee40f756b9e55656fe2e744bc... to /tmp/review/std_msgs-0.4.11-de0dcf1.tar.gz % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 154 100 154 0 0 122 0 0:00:01 0:00:01 --:--:-- 123 100 6947 100 6947 0 0 3041 0 0:00:02 0:00:02 --:--:-- 19569 67999742fb86f0ed7b2880e5917ddf5a /tmp/review/std_msgs-0.4.11-de0dcf1.tar.gz 67999742fb86f0ed7b2880e5917ddf5a /home/asinha/rpmbuild/SOURCES/std_msgs-0.4.11-de0dcf1.tar.gz removed ‘/tmp/review/std_msgs-0.4.11-de0dcf1.tar.gz’ removed directory: ‘/tmp/review’ [asinha@localhost SRPMS]$
[+] Build succeeds on at least one primary arch [+] Build succeeds on all primary arches or has ExcludeArch + bugs filed [+] BuildRequires correct, justified where necessary [-] Locales handled with %find_lang, not %_datadir/locale/* [+] %post, %postun call ldconfig if package contains shared .so files [+] No bundled libs [-] Relocatability is justified [+] Package owns all directories it creates [?] Package requires others for directories it uses but does not own [asinha@localhost result]$ review-req-check == ros-std_msgs-0.4.11-2.20130605gitde0dcf1.fc20.noarch.rpm == Provides: ros-std_msgs = 0.4.11-2.20130605gitde0dcf1.fc20
Requires: python(abi) = 2.7 ros-release
== ros-std_msgs-0.4.11-2.20130605gitde0dcf1.fc20.src.rpm == Provides:
Requires: cmake python-setuptools-devel catkin-devel python-genmsg-devel python-gencpp-devel python-genlisp-devel python-genpy-devel
== ros-std_msgs-devel-0.4.11-2.20130605gitde0dcf1.fc20.noarch.rpm == Provides: pkgconfig(std_msgs) = 0.4.11 ros-std_msgs = 0.4.11-2.20130605gitde0dcf1.fc20 ros-std_msgs-devel = 0.4.11-2.20130605gitde0dcf1.fc20
Requires: /usr/bin/pkg-config
[asinha@localhost result]$
^^ 1.Just confirming: Which package that is Required by this one is the %{_datadir}/common-lisp/ros/ directory owned by? 2. Shouldn't the package require the non devel versions of the python BRs to function?
[+] No duplication in %files unless necessary for license files [+] File permissions are sane [+] Package contains permissible code or content [-] Large docs go in -doc subpackage [?] %doc files not required at runtime There is no documentation at all. No licence or even a README :/
[-] Static libs go in -static package/virtual Provides [+] Development files go in -devel package [+] -devel packages Require base with fully-versioned dependency, %_isa Noarch so isa isn't needed
[+] No .la files [-] GUI app uses .desktop file, installs it with desktop-file-install [-] File list does not conflict with other packages' without justification [+] File names are valid UTF-8
** Optional review guidelines: ** [?] Query upstream about including license files We can, but I don't think ROS intends to include licence files in any of it's packages. Should we make ros-release include a license file if it doesn't already, since all these packages will be expected to Require it?
[-] Translations of description, summary [+] Builds in mock [+] Builds on all arches [-] Functions as described (e.g. no crashes) [-] Scriptlets are sane [+] Subpackages require base with fully-versioned dependency if sensible [+] .pc file subpackage placement is sensible [+] No file deps outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin [-] Include man pages if available
Naming guidelines: [+] Package names use only a-zA-Z0-9-._+ subject to restrictions on -._+ [+] Package names are sane [+] No naming conflicts [+] Spec file name matches base package name [+] Version is sane [+] Version does not contain ~ [+] Release is sane [+] %dist tag [+] Case used only when necessary [-] Renaming handled correctly
Packaging guidelines: [+] Useful without external bits [+] No kmods [-] Pre-built binaries, libs removed in %prep [+] Sources contain only redistributable code or content [+] Spec format is sane [+] Package obeys FHS, except libexecdir, /run, /usr/target [+] No files in /bin, /sbin, /lib* on >= F17 [-] Programs run before FS mounting use /run instead of /var/run [-] Binaries in /bin, /sbin do not depend on files in /usr on < F17 [-] No files under /srv, /opt, /usr/local [+] Changelog in prescribed format [+] No Packager, Vendor, Copyright, PreReq tags [+] Summary does not end in a period [-] Correct BuildRoot tag on < EL6 [-] Correct %clean section on < EL6 [?] Requires correct, justified where necessary The directory owning the list sub dir needs clarification
[+] Summary, description do not use trademarks incorrectly [-] All relevant documentation is packaged, appropriately marked with %doc [-] Doc files do not drag in extra dependencies (e.g. due to +x) [-] Code compilable with gcc is compiled with gcc [+] Build honors applicable compiler flags or justifies otherwise [-] PIE used for long-running/root daemons, setuid/filecap programs [+] Useful -debuginfo package or disabled and justified [-] Package with .pc files Requires pkgconfig on < EL6 [+] No static executables [+] Rpath absent or only used for internal libs [+] Config files marked with %config(noreplace) or justified %config [+] No config files under /usr [-] Third party package manager configs acceptable, in %_docdir [-] .desktop files are sane [+] Spec uses macros consistently [+] Spec uses macros instead of hard-coded names where appropriate [+] Spec uses macros for executables only when configurability is needed [+] %makeinstall used only when alternatives don't work [+] Macros in Summary, description are expandable at srpm build time [+] Spec uses %{SOURCE#} instead of $RPM_SOURCE_DIR and %sourcedir [+] No software collections (scl) [+] Macro files named /etc/rpm/macros.%name [+] Build uses only python/perl/shell+coreutils/lua/BuildRequired langs [+] %global, not %define [-] Package translating with gettext BuildRequires it [-] Package translating with Linguist BuildRequires qt-devel [+] File ops preserve timestamps [+] Parallel make [+] No Requires(pre,post) notation [-] User, group creation handled correctly (See Packaging:UsersAndGroups) [-] Web apps go in /usr/share/%name, not /var/www [-] Conflicts are justified [+] One project per package [+] No bundled fonts [-] Patches have appropriate commentary [-] Available test suites executed in %check [-] tmpfiles.d used for /run, /run/lock on >= F15
** Python guidelines: ** [?] Runtime Requires correct Should this package require other ros python packages, the non devel versions of ones required as BRs? [-] Python macros declared on < EL6 [+] All .py files packaged with .pyc, .pyo counterparts [?] Includes .egg-info files/directories when generated Should egg info be generated for this package? [-] Provides/Requires properly filtered [-] Code that invokes gtk.gdk.get_pixels_array() Requires numpy
Are all the files in the non devel package files that are required at runtime? I think they are. Do you think you need to split the package further in to a python and a lisp subpackage? Then we'd have:
a main package a python bindings package a list package a c++ headers package
The package is mostly OK. Some tiny issues that need to be handled.
https://bugzilla.redhat.com/show_bug.cgi?id=928584
--- Comment #7 from Rich Mattes richmattes@gmail.com --- Basically, the python and lisp compontents are the generated messages. They are required during runtime for any python or lisp program that needs the messages, though not every ROS program will need one or both of them. I think it would be possible to split them out into something like package-python-msgs and package-lisp-msgs, but I'm not convinced it's worth the effort.
As far as /usr/share/common-lisp/ros goes, I'm not sure we can use that folder as per [1]. I think I may have to change the way that catkin is installing the lisp messages so that it lands in a packagename subfolder (or maybe we can just move the ros/ folder into common-lisp/source and have ros-release own it)
You're right about requiring the runtime genpy and genlisp libraries: all the python getnpy messages import genpy. I'm not sure about lisp, and it looks like the c++ generated headers require headers from roscpp_core-devel. I'll update the Requires accordingly.
I don't think egg-info needs to be generated; that guideline is only for when upstream does generate it, which isn't the case here.
[1] http://fedoraproject.org/wiki/Packaging:Lisp#Install_location_and_hooking_in...
https://bugzilla.redhat.com/show_bug.cgi?id=928584
--- Comment #8 from Rich Mattes richmattes@gmail.com --- Update:
Spec URL: http://rmattes.fedorapeople.org/rospackages/std-msgs/ros-std_msgs.spec SRPM URL: http://rmattes.fedorapeople.org/rospackages/std-msgs/ros-std_msgs-0.4.11-3.2...
I tried to address the concerns you mentioned above, and I also tried to conform to the common-lisp packaging guidelines
$ rpmlint ros-std_msgs.spec ../../RPMS/noarch/ros-std_msgs*-0.4.11-3* ros-std_msgs.noarch: W: spelling-error %description -l en_US msgs -> mags, megs, mugs ros-std_msgs.noarch: W: spelling-error %description -l en_US multiarrays -> multiracial ros-std_msgs.noarch: W: no-documentation ros-std_msgs.noarch: E: zero-length /usr/share/std_msgs/msg/Empty.msg ros-std_msgs-devel.noarch: W: spelling-error Summary(en_US) msgs -> mags, megs, mugs ros-std_msgs-devel.noarch: W: spelling-error %description -l en_US msgs -> mags, megs, mugs ros-std_msgs-devel.noarch: W: no-documentation 2 packages and 1 specfiles checked; 1 errors, 6 warnings.
https://bugzilla.redhat.com/show_bug.cgi?id=928584
Bug 928584 depends on bug 927473, which changed state.
Bug 927473 Summary: Review Request: python-gencpp - C++ ROS message and service generators https://bugzilla.redhat.com/show_bug.cgi?id=927473
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Resolution|--- |ERRATA
https://bugzilla.redhat.com/show_bug.cgi?id=928584
Bug 928584 depends on bug 927475, which changed state.
Bug 927475 Summary: Review Request: python-genlisp - Lisp ROS message and service generators https://bugzilla.redhat.com/show_bug.cgi?id=927475
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Resolution|--- |ERRATA
https://bugzilla.redhat.com/show_bug.cgi?id=928584
Bug 928584 depends on bug 927478, which changed state.
Bug 927478 Summary: Review Request: python-genpy - Python ROS message and service generation https://bugzilla.redhat.com/show_bug.cgi?id=927478
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Resolution|--- |ERRATA
https://bugzilla.redhat.com/show_bug.cgi?id=928584
Christopher Meng cickumqt@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |cickumqt@gmail.com Flags| |needinfo?(sanjay.ankur@gmai | |l.com)
--- Comment #9 from Christopher Meng cickumqt@gmail.com --- This ticket has blocked another review for a long time.
https://bugzilla.redhat.com/show_bug.cgi?id=928584
Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(sanjay.ankur@gmai | |l.com) |
--- Comment #10 from Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com --- You're welcome to take over and continue the review.
https://bugzilla.redhat.com/show_bug.cgi?id=928584
Christopher Meng cickumqt@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC|package-review@lists.fedora | |project.org | Assignee|sanjay.ankur@gmail.com |cickumqt@gmail.com
--- Comment #11 from Christopher Meng cickumqt@gmail.com --- 0.5.8 is its latest version, right?
https://github.com/ros/std_msgs/releases
https://bugzilla.redhat.com/show_bug.cgi?id=928584 Bug 928584 depends on bug 927462, which changed state.
Bug 927462 Summary: Review Request: roscpp_core - The ROS C++ API https://bugzilla.redhat.com/show_bug.cgi?id=927462
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution|--- |WONTFIX
https://bugzilla.redhat.com/show_bug.cgi?id=928584
Rich Mattes richmattes@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution|--- |WONTFIX Last Closed| |2018-02-07 19:47:03
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com -
Red Hat Bugzilla