Bug 998141

(In reply to Rich Mattes from comment #0)
>
> Description: 
> The Simulation Description Format (SDF) is an XML file format used to
> describe all the elements in a simulation environment.

I don't understand this description. What kind of simulation environment does the term "simulation environment" refer to? I'd guess it refers to some special simulation SW suite or package?

Also, I find the name "sdformat" to be misleading. The name "sdformat" to me suggests, this tool was an "SD Card" formatter.

It's part of the gazebo project, so it refers to a robotic simulation.  I'll update the description to be a little bit better.  There's not a whole lot of explaination from upstream about what it does (the whole library seems like more of an implementation detail for gazebo) but I've come up with something.

Spec URL: http://rmattes.fedorapeople.org/RPMS/sdformat/sdformat.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/sdformat/sdformat-1.4.5-2.fc19.src.rpm

I can see how the name can be confusing, but there's not a whole lot I can do about that.  That's what upstream calls it, and what the tarball is being called, etc.  The only other alternative I can think of is "sdf", but that seems worse[1]

[1] http://en.wikipedia.org/wiki/SDF

(In reply to Rich Mattes from comment #2)
> It's part of the gazebo project, so it refers to a robotic simulation.  I'll
> update the description to be a little bit better.  There's not a whole lot
> of explaination from upstream about what it does (the whole library seems
> like more of an implementation detail for gazebo) but I've come up with
> something.
> 
> Spec URL: http://rmattes.fedorapeople.org/RPMS/sdformat/sdformat.spec
> SRPM URL:
> http://rmattes.fedorapeople.org/RPMS/sdformat/sdformat-1.4.5-2.fc19.src.rpm
Fine with me.

> I can see how the name can be confusing, but there's not a whole lot I can
> do about that.  That's what upstream calls it, and what the tarball is being
> called, etc.  The only other alternative I can think of is "sdf", but that
> seems worse[1]
How about calling the package gazebo-sdformat or osrf-sdformat?

I'm a little uneasy about changing the name, because the package naming guidelines[1] state that we should be using the upstream project/tarball name for the package.  I can't find any guidelines about making sure a package name isn't too confusing or too general; the only thing that the guidelines mandate is that package names do not conflict.

[1] http://fedoraproject.org/wiki/Packaging:NamingGuidelines

Updated to version 1.4.8:

Spec URL: http://rmattes.fedorapeople.org/RPMS/sdformat/sdformat.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/sdformat/sdformat-1.4.8-1.fc19.src.rpm

rpmlint:
$ rpmlint ../RPMS/x86_64/sdformat* sdformat.spec 
sdformat.x86_64: W: spelling-error %description -l en_US sdf -> sf, SD
3 packages and 1 specfiles checked; 0 errors, 1 warnings.

Hi Rich,
sdformat seems in a pretty good shape. Some minor concerns:

* The documentations seems to be over 2 mb and 445 fiels. Maybe it's worth to make a doc subpackage
* I notice it uses some header in src/urdf that seems similar to what is provided by urdfdom-headers-devel-0.2.3-1.fc19.noarch and have a BSD license. Maybe you can drop that and require urdfdom-headers-devel.

@Christopher, is it okay for you if I do the review?

Best,

Mario

Yes, I'm still on holiday now.

Okay, I'll review it!

Please Rich, answer my previous comments and I'll continue with the formal review.

Hello Rich,
did you have the time to review my previous comments?

Best,

Mario

I did.  I pulled out the urdf stuff, but some of the unit tests started failing.  I need to figure out what's going on there.

Alright, updated packages are here:

Spec URL: http://rmattes.fedorapeople.org/RPMS/sdformat/sdformat.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/sdformat/sdformat-1.4.8-2.fc19.src.rpm

rpmlint:
$ rpmlint sdformat.spec ../RPMS/x86_64/sdformat*1.4.8-2*
sdformat.x86_64: W: spelling-error %description -l en_US sdf -> sf, SD
3 packages and 1 specfiles checked; 0 errors, 1 warnings.

I solved the unit test failures by updating the "urdfdom" package to the latest upstream release (0.2.9).  I'm submitting a buildroot override and bodhi update tonight, so you will be able to build the sdformat package on koji, and locally by grabbing the latest urdfdom packages from updates-testing.

That's great, Rich, I'll test it tomorrow and finish the review

(In reply to Mario Ceresa from comment #12)
> That's great, Rich, I'll test it tomorrow and finish the review

2 weeks after your tomorrow. ping.

Hi Christopher, thanks for the ping. I forgot about this review :)

Rich, I had two small problems:
* You seem to miss a BR for tinyxml-devel
* Test INTEGRATION_fixed_joint_reduction failed on my machine (OTHER_FAULT). If you feel that it's something that requires upstream intervention, you might want to make the tests informative only for now (ie: make tests || exit 0)

Could you also post a koji scratch-build link?

Best,

Mario

Updated to version 1.4.10

Spec URL: http://rmattes.fedorapeople.org/RPMS/sdformat/sdformat.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/sdformat/sdformat-1.4.10-1.fc20.src.rpm

scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=6188236

rpmlint:
$ rpmlint sdformat.spec ../RPMS/x86_64/sdformat*
sdformat.x86_64: W: spelling-error %description -l en_US sdf -> sf, SD
3 packages and 1 specfiles checked; 0 errors, 1 warnings.

I added a BuildRequires on tinyxml-devel, and the unit tests are now passing for me with release 1.4.10 but they're failing on koji for i686.  I've made them informational for now while I figure out what's going on.

Here it goes the formal review. In summary:

* Please create a sub package for the documentation
* Please unbundle the gtest framework included in test/ 


Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- Large documentation must go in a -doc subpackage. Large could be size (~1MB)
  or number of files.
  Note: Documentation size is 2508800 bytes in 443 files.
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#PackageDocumentation

---> Please do.

===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig called in %post and %postun if required.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "BSD (3 clause)", "Apache (v2.0)", "Unknown or generated". 7 files have
     unknown license. Detailed output of licensecheck in
     /home/mario/fedora/998141-sdformat/licensecheck.txt

---> !You bundle gtest! Please remove it and use fedora's shipped one
 
[x]: License file installed when any subpackage combination is installed.
[x]: %build honors applicable compiler flags or justifies otherwise.
[!]: Package contains no bundled libraries without FPC exception.
---> Bundles gtest

[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[!]: Uses parallel make %{?_smp_mflags} macro.
---> False positive. It uses parallel make indeed

[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[x]: Scriptlets must be sane, if used.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[!]: %check is present and all tests pass.
---> Tests are informative only for now. 

[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Fully versioned dependency in subpackages if applicable.
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[-]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
     Note: Arch-ed rpms have a total of 3133440 bytes in /usr/share
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: sdformat-1.4.10-1.fc20.x86_64.rpm
          sdformat-devel-1.4.10-1.fc20.x86_64.rpm
          sdformat-1.4.10-1.fc20.src.rpm
sdformat.x86_64: W: spelling-error %description -l en_US sdf -> sf, SD
sdformat.src: W: spelling-error %description -l en_US sdf -> sf, SD
3 packages and 0 specfiles checked; 0 errors, 2 warnings.


Rpmlint (installed packages)
----------------------------
# rpmlint sdformat sdformat-devel
sdformat.x86_64: W: spelling-error %description -l en_US sdf -> sf, SD
sdformat.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsdformat.so.1.4.10 /lib64/libboost_program_options.so.1.54.0
sdformat.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsdformat.so.1.4.10 /lib64/libboost_iostreams.so.1.54.0
sdformat.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsdformat.so.1.4.10 /lib64/liburdfdom_sensor.so.0.2
sdformat.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsdformat.so.1.4.10 /lib64/liburdfdom_model_state.so.0.2
sdformat.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsdformat.so.1.4.10 /lib64/liburdfdom_world.so.0.2
sdformat.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsdformat.so.1.4.10 /lib64/libconsole_bridge.so.0
2 packages and 0 specfiles checked; 0 errors, 7 warnings.
# echo 'rpmlint-done:'



Requires
--------
sdformat (rpmlib, GLIBC filtered):
    /sbin/ldconfig
    libboost_filesystem.so.1.54.0()(64bit)
    libboost_iostreams.so.1.54.0()(64bit)
    libboost_program_options.so.1.54.0()(64bit)
    libboost_regex.so.1.54.0()(64bit)
    libboost_system.so.1.54.0()(64bit)
    libc.so.6()(64bit)
    libconsole_bridge.so.0()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgcc_s.so.1(GCC_4.0.0)(64bit)
    libm.so.6()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libtinyxml.so.0()(64bit)
    liburdfdom_model.so.0.2()(64bit)
    liburdfdom_model_state.so.0.2()(64bit)
    liburdfdom_sensor.so.0.2()(64bit)
    liburdfdom_world.so.0.2()(64bit)
    rtld(GNU_HASH)

sdformat-devel (rpmlib, GLIBC filtered):
    /usr/bin/pkg-config
    cmake
    libsdformat.so.1()(64bit)
    sdformat(x86-64)



Provides
--------
sdformat:
    libsdformat.so.1()(64bit)
    sdformat
    sdformat(x86-64)

sdformat-devel:
    pkgconfig(sdformat)
    sdformat-devel
    sdformat-devel(x86-64)



Source checksums
----------------
https://bitbucket.org/osrf/sdformat/get/sdformat_1.4.10.tar.bz2 :
  CHECKSUM(SHA256) this package     : 9fdad1652c7f5f5d33a0bd494d3fc71a500794392e56dfe1ff9c991c089b3117
  CHECKSUM(SHA256) upstream package : 9fdad1652c7f5f5d33a0bd494d3fc71a500794392e56dfe1ff9c991c089b3117


Generated by fedora-review 0.5.0 (920221d) last change: 2013-08-30
Command line :/usr/bin/fedora-review -b 998141
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, SugarActivity, Perl, R, PHP, Ruby
Disabled flags: EPEL5, EXARCH, DISTTAG

Thanks!  I've updated the package to address your concerns:

Spec URL: http://rmattes.fedorapeople.org/RPMS/sdformat/sdformat.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/sdformat/sdformat-1.4.10-2.fc20.src.rpm

$ rpmlint sdformat.spec ../RPMS/x86_64/sdformat*
sdformat.x86_64: W: spelling-error %description -l en_US sdf -> sf, SD
sdformat-devel.x86_64: W: no-documentation
4 packages and 1 specfiles checked; 0 errors, 2 warnings.

scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=6196295

I see no more problems, so the package is
APPROVED

Great work Rich!

Thanks for the review Mario!

New Package SCM Request
=======================
Package Name: sdformat
Short Description: The Simulation Description Format
Owners: rmattes
Branches: f18 f19 f20 el6
InitialCC:

Git done (by process-git-requests).

sdformat-1.4.11-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/sdformat-1.4.11-1.fc20

sdformat-1.4.11-1.fc20 has been pushed to the Fedora 20 testing repository.

sdformat-1.4.11-1.fc20 has been pushed to the Fedora 20 stable repository.