Bug 871204 (urdfdom-headers) - Review Request: urdfdom-headers - The URDF (U-Robot Description Format) headers
Summary: Review Request: urdfdom-headers - The URDF (U-Robot Description Format) headers
Keywords:
Status: CLOSED ERRATA
Alias: urdfdom-headers
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ankur Sinha (FranciscoD)
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 871205
TreeView+ depends on / blocked
 
Reported: 2012-10-29 22:18 UTC by Rich Mattes
Modified: 2013-04-20 20:20 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-04-20 20:20:14 UTC
sanjay.ankur: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Rich Mattes 2012-10-29 22:18:46 UTC
Spec URL: http://rmattes.fedorapeople.org/RPMS/urdfdom-headers/urdfdom-headers.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/urdfdom-headers/urdfdom-headers-0.2.1-1.fc17.src.rpm
Description: 
The URDF (U-Robot Description Format) headers provides core data structure
headers for URDF.

For now, the details of the URDF specifications reside on
http://ros.org/wiki/urdf

Fedora Account System Username: rmattes

Comment 1 Ralf Corsepius 2012-10-30 07:02:01 UTC
The licensing claimed in the file LICENSE is does not match with the licenses found inside of the sources:

# find -name '*.h' | xargs grep -i license
./urdf_world/include/urdf_world/world.h:* Software License Agreement (BSD License)
./urdf_model/include/urdf_model/color.h:* Software License Agreement (BSD License)
./urdf_model/include/urdf_model/twist.h:* Software License Agreement (BSD License)
./urdf_model/include/urdf_model/link.h:* Software License Agreement (BSD License)
./urdf_model/include/urdf_model/model.h:* Software License Agreement (BSD License)
./urdf_model/include/urdf_model/joint.h:* Software License Agreement (BSD License)
./urdf_model/include/urdf_model/pose.h:* Software License Agreement (BSD License)
./urdf_sensor/include/urdf_sensor/sensor.h:* Software License Agreement (BSD License)
./urdf_model_state/include/urdf_model_state/model_state.h:* Software License Agreement (BSD License)

Besides this, consider taking into account what I wrote in the thread starting at http://lists.fedoraproject.org/pipermail/packaging/2012-October/008714.html

Admitted, as this package doesn't provide any testsuites nor coding examples, applying this proposal to this package is pretty meaningless and raises doubts on this package's quality.

Comment 2 Rich Mattes 2012-12-02 22:04:04 UTC
Bug filed upstream about licensing:
https://bitbucket.org/osrf/urdfdom_headers/issue/1/license-file-doesnt-match-licenses-in

I don't think that proposal applies to this package, since nothing is actually built.  It's just providing definitions for the URDF standard.  Lack of unit tests is something to take up with upstream, but since there is at least one package that requires these headers it's probably a safe bet that anything broken in these headers will be revealed when its dependencies are built.

Comment 3 Rich Mattes 2013-03-21 02:36:16 UTC
Updated to latest upstream version:

Spec URL: http://rmattes.fedorapeople.org/RPMS/urdfdom-headers/urdfdom-headers.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/urdfdom-headers/urdfdom-headers-0.2.2-1.fc18.src.rpm


rpmlint output:
$ rpmlint urdfdom-headers.spec ../RPMS/noarch/urdfdom-headers-devel-0.2.2-1.fc18.noarch.rpm 
urdfdom-headers.spec: W: invalid-url Source0: urdfdom-headers-0.2.2.tar.bz2
1 packages and 1 specfiles checked; 0 errors, 1 warnings.

The LICENSE file was changed to BSD after 0.2.2 was tagged.  I removed the LICENSE file from %doc until it is corrected in the next release

Comment 4 Ankur Sinha (FranciscoD) 2013-04-09 13:43:51 UTC
Hi Rich,

Review:

[+] OK
[-] NA
[?] Issue

** Mandatory review guidelines: **
 [+] rpmlint output:
[ankur@localhost  SRPMS]$ rpmlint ../SPECS/urdfdom-headers.spec
urdfdom-headers-0.2.2-1.fc18.src.rpm
/var/lib/mock/fedora-rawhide-x86_64/result/*.rpm
../SPECS/urdfdom-headers.spec: W: invalid-url Source0:
urdfdom-headers-0.2.2.tar.bz2
urdfdom-headers.src: W: invalid-url Source0: urdfdom-headers-0.2.2.tar.bz2
urdfdom-headers.src: W: invalid-url Source0: urdfdom-headers-0.2.2.tar.bz2
3 packages and 1 specfiles checked; 0 errors, 3 warnings.
[ankur@localhost  SRPMS]$
^^
ALL OK

 [+] License is acceptable (...)
 [+] License field in spec is correct
 [-] License files included in package %docs if included in source package
^ 
Skipped for the time being

 [-] 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
   Upstream SHA256: ...
   Your SHA256:     ...
^ 
Generated from hg clone. Script attached. 

 [+] 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
 [+] 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
 [-] Static libs go in -static package/virtual Provides
 [+] Development files go in -devel package
 [-] -devel packages Require base with fully-versioned dependency, %_isa
^^
Unneeded. The main package is empty

 [+] 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
^ 
Issue filed

 [-] 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
 [-] 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
^^
Please add a comment explaining the patch
 [-] Available test suites executed in %check
 [-] tmpfiles.d used for /run, /run/lock on >= F15


Looks good. No blockers. A few tiny issues only. 


Just one query before I approve it:
Since it's a header only package, should it provide a static package Rich?

Package is mostly ready for approval.

Thanks,
Warm regards,
Ankur

Comment 5 Rich Mattes 2013-04-09 23:31:54 UTC
Update:
Spec URL: http://rmattes.fedorapeople.org/RPMS/urdfdom-headers/urdfdom-headers.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/urdfdom-headers/urdfdom-headers-0.2.2-2.fc18.src.rpm

You're right about the static provides, I went ahead and added it to the -devel subpackage.  I also moved the package description to the -devel subpackage, since the base package isn't built.

rpmlint:
$ rpmlint urdfdom-headers.spec ../RPMS/noarch/urdfdom-headers-devel-0.2.2-2.fc18.noarch.rpm 
urdfdom-headers.spec: W: invalid-url Source0: urdfdom-headers-0.2.2.tar.bz2
1 packages and 1 specfiles checked; 0 errors, 1 warnings.

Comment 6 Ankur Sinha (FranciscoD) 2013-04-10 10:15:00 UTC
Hi Rich,

Errors have been fixed. Static provide provided.


XXX APPROVED XXX

Thanks,
Warm regards,
Ankur

Comment 7 Rich Mattes 2013-04-10 13:01:43 UTC
New Package SCM Request
=======================
Package Name: urdfdom-headers
Short Description: The URDF (U-Robot Description Format) headers
Owners: rmattes
Branches: f17 f18 f19 el6
InitialCC:

Comment 8 Gwyn Ciesla 2013-04-10 13:08:12 UTC
Git done (by process-git-requests).

Comment 9 Fedora Update System 2013-04-12 13:04:10 UTC
urdfdom-headers-0.2.2-2.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/urdfdom-headers-0.2.2-2.fc19

Comment 10 Fedora Update System 2013-04-12 15:18:51 UTC
urdfdom-headers-0.2.2-2.fc19 has been pushed to the Fedora 19 testing repository.

Comment 11 Fedora Update System 2013-04-20 20:20:16 UTC
urdfdom-headers-0.2.2-2.fc19 has been pushed to the Fedora 19 stable repository.


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