Bug 448025 (player)

Summary: Review Request: player - Cross-platform robot device interface and server
Product: [Fedora] Fedora Reporter: Tim Niemueller <tim>
Component: Package ReviewAssignee: Jef Spaleta <jspaleta>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, notting, rainwoodman
Target Milestone: ---Flags: jspaleta: fedora‑review+
kevin: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2008-09-03 07:16:01 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 452486    
Attachments:
Description Flags
modified spec file to fix review issues
none
rpmlint output
none
Patch to spec file with review changes none

Description Tim Niemueller 2008-05-22 20:18:15 EDT
Spec URL: http://fedorapeople.org/~timn/player/player.spec
SRPM URL: http://fedorapeople.org/~timn/player/player-2.1.0-0.2.rc2.fc9.src.rpm
Description:
Player is a network server for robot control. Running on your robot, Player
provides a clean and simple interface to the robot's sensors and actuators
over the IP network. Your client program talks to Player over a TCP socket,
reading data from sensors, writing commmands to actuators, and configuring
devices on the fly. Player supports a variety of robot hardware.

URL: http://playerstage.sourceforge.net
Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=625488
Comment 1 Jef Spaleta 2008-06-23 11:47:30 EDT
I'm working on the review today. Sorry for the delay. 

-jef
Comment 2 Jef Spaleta 2008-06-23 13:24:46 EDT
Okay first comments.

Builds against f9 and devel in mock.

Doesn't build against F8 in mock .. this isn't a blocker. But you should be
aware of this in case you want to open an F8 branch.  If you are planning on
opening an F8 branch we can look at this more closely after the review is done.


I think you should move the example code into an examples directory under 
/usr/share/ instead of leaving them under /usr/lib/. 
Either /usr/share/doc/player-examples by treating them as docs or under or
/usr/share/player/ by treating them as normal file payloads.  
There are existing examples of both types of behavior for examples subpackages.


I've run rpmlint against the f9 builds.. and for the most part no serious
problems.  But... the debuginfo package is throwing a lot of warnings and errors
from rpmlint concerning files in /usr/src/*  due to the fact that they are set
as executable in the orginal source tarball. I don't think its a serious issue,
but it might be easily fixable in the spec with a recursive chmod call to strip
the executable bits in the source subdirectories tree before the build. I'll
probably patch that into the spec file.

Working through the formal review now. I'll post a patched spec file as part of
the formal review.

-jef

Comment 3 Jason Tibbitts 2008-06-23 13:39:23 EDT
I was going to comment that /usr/bin/player is a terribly generic name which
would conflict with a few things which aren't in our distro (the last.fm player
and mplayer are two examples which other distros install as /usr/bin/player).

However, when building this to see what binaries it actually installed, the
build failed:

 g++ -DHAVE_CONFIG_H -I. -I../../.. -I../../../libplayercore
-I../../../client_libs/libplayerc++ -I../../../client_libs/libplayerc -Wall
-I../../.. -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
-fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -MT dbconn.lo
-MD -MP -MF .deps/dbconn.Tpo -c dbconn.cc  -fPIC -DPIC -o .libs/dbconn.o
dbconn.cc: In member function 'BoundingBox PostgresConn::BinaryToBBox(const
uint8_t*, uint32_t)':
dbconn.cc:317: error: invalid conversion from 'const GEOSGeometry*' to
'GEOSGeometry*'
dbconn.cc:323: error: invalid conversion from 'const GEOSCoordSequence*' to
'GEOSCoordSequence*'
make[4]: *** [dbconn.lo] Error 1

Jef, did your successful builds happen on a 64-bit machine?  I'm building in
mock on x86_64 rawhide.
Comment 4 Jef Spaleta 2008-06-23 16:32:37 EDT
no 64 build here. I have it building under f9 32bit.

The project itself is actually named player so then binaries of the form
/usr/bin/player*  aren't in shorthand.  Is it appropriate to rename binaries
when the actual project name is too generic? And what exactly do you do to make
them less generic?  its one thing to make a binary be more specific to match the
project name.. requiring mplayer's binary to say mplayer instead of player. But
when the project name is too generic what exactly do you do?  Do you append
player- to all the binaries so we have executables like player-player and
player-playernav ?

-jef


-jef
Comment 5 Jef Spaleta 2008-06-23 18:52:26 EDT
Okay you'll need to include  .desktop files for each of the gui applications or
drop a comment indicating why the guis do not need to be in the menus.
You would need individual .desktop files for these three commands:
playerv
playernav
playercam

This is a blocker.

Also seriously consider renaming player executable to player_server or equivalent.

-jef

Comment 6 Jef Spaleta 2008-06-23 20:33:10 EDT
rpmlint output with specfile modified to silence the debuginfo rpmlint warnings.
See attached attachment for updated specfile.

I'm not sure what to do about the examples subpackage.  It contains source code,
libraries and executables..all of which is put under _libdir/player/examples/

I'm not sure that is appropriate. It feels a little wonky to do it that way.
If the point of the examples are coding examples for review...I would almost
prefer that the examples were not compiled and that the examples subpackage only
included the source material and instructions on how to compile the examples on
a client system as needed.  

Anyways here's the rpmlint output. I'm fine with everything except the examples
warnings and docs subpackage errors. I'll need to look at the docs errors a bit
more.  Do those mentioned zero-length map files need to be included at all?


player-devel.i386: W: no-documentation

player-doc.i386: E: zero-length
/usr/share/doc/player-doc-2.1.0/player-docs/classClaser__coll__graph.map
player-doc.i386: E: zero-length
/usr/share/doc/player-doc-2.1.0/player-docs/classMessageReplaceRule__coll__graph.map
player-doc.i386: E: zero-length
/usr/share/doc/player-doc-2.1.0/player-docs/classConfigFile__coll__graph.map

player-examples.i386: W: no-documentation
player-examples.i386: W: devel-file-in-non-devel-package
/usr/lib/player/examples/libplayerc++/args.h
player-examples.i386: W: devel-file-in-non-devel-package
/usr/lib/player/examples/plugins/exampleinterface/example_xdr.h
player-examples.i386: W: devel-file-in-non-devel-package
/usr/lib/player/examples/plugins/exampledriver/libexampledriver.a
player-examples.i386: W: devel-file-in-non-devel-package
/usr/lib/player/examples/plugins/multidriver/libmultidriver.a
player-examples.i386: W: devel-file-in-non-devel-package
/usr/lib/player/examples/plugins/opaquedriver/libopaquedriver.a
player-examples.i386: W: devel-file-in-non-devel-package
/usr/lib/player/examples/plugins/exampleinterface/example_functiontable.c
player-examples.i386: W: devel-file-in-non-devel-package
/usr/lib/player/examples/plugins/exampleinterface/libeginterf.a
player-examples.i386: W: devel-file-in-non-devel-package
/usr/lib/player/examples/plugins/exampleinterface/eginterf_client.c
player-examples.i386: W: devel-file-in-non-devel-package
/usr/lib/player/examples/plugins/logplugin/liblogplugin.a
player-examples.i386: W: devel-file-in-non-devel-package
/usr/lib/player/examples/plugins/exampleinterface/libeginterfdriver.a
player-examples.i386: W: devel-file-in-non-devel-package
/usr/lib/player/examples/plugins/exampleinterface/example_interface.h
player-examples.i386: W: devel-file-in-non-devel-package
/usr/lib/player/examples/libplayerc/speech_c_client.c
player-examples.i386: W: devel-file-in-non-devel-package
/usr/lib/player/examples/libplayerc/simple.c
player-examples.i386: W: devel-file-in-non-devel-package
/usr/lib/player/examples/libplayerc/service_discovery.c
player-examples.i386: W: devel-file-in-non-devel-package
/usr/lib/player/examples/libplayerc/vmap.c

player-python.i386: W: no-documentation
player-python.i386: E: non-standard-executable-perm
/usr/lib/python2.5/site-packages/_playerc.so 0775

player-static.i386: W: no-documentation
8 packages and 0 specfiles checked; 4 errors, 19 warnings.
Comment 7 Jef Spaleta 2008-06-23 20:39:03 EDT
Created attachment 310088 [details]
modified spec file to fix review issues

spec file to fix review related issues.

Please see full srpm at
http://jspaleta.fedorapeople.org/reviews/player/player-2.1.0-0.3.rc2.fc8.src.rpm


for specfile and .desktop sources

The .desktop files were added for each of the gui applications. The Category
setting may need to be adjusted. They currently configured to have menu entries
show up under Education. 

-jef
Comment 8 Jason Tibbitts 2008-06-23 20:44:22 EDT
FYI, here's a failing scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=677252

We don't have any hard rules about renaming binaries; packages in the distro
should not conflict with each other if at all possible (and should always do so
explicitly) but nothing provides /usr/bin/player so there won't be any
conflicts. It does, however, pay to be wary of things that might potentially
conflict, and I don't think anyone would disagree that /usr/bin/player is a
terribly generic name.  So it's really up to the maintainer in this case.
Comment 9 Patrice Dumas 2008-06-24 02:59:36 EDT
(In reply to comment #4)
> no 64 build here. I have it building under f9 32bit.
> 
> The project itself is actually named player so then binaries of the form
> /usr/bin/player*  aren't in shorthand.  Is it appropriate to rename binaries
> when the actual project name is too generic? 

It is, but it is also possible to convince upstream in some cases.
Comment 10 Jef Spaleta 2008-06-24 03:27:48 EDT
(In reply to comment #8)
> FYI, here's a failing scratch build:
> http://koji.fedoraproject.org/koji/taskinfo?taskID=677252

There seems to have been a significant change in the geos package between fedora
9 and development that has changed the api.  The player code will need to be
patched to support the newer geos currently available in the development tree.
This might take a little bit of investigation to fix... or it might be a
quickie.  I'll need to rebuild the geos in development onto my f9 box to do some
testing.

It appears that geos was never built in the f8 or in the f9 timeframe, and both
trees are still using the same geos version built against f7.

-jef
Comment 11 Jef Spaleta 2008-06-24 15:08:52 EDT
(In reply to comment #8)
> FYI, here's a failing scratch build:
> http://koji.fedoraproject.org/koji/taskinfo?taskID=677252

I understand the failure against development.  Did you also have a failing
building on 64bit against f9?

-jef
Comment 12 Jef Spaleta 2008-06-27 13:14:38 EDT
Just FYI, I'm looking into the development build failure.  I'm pretty sure I can
patch a fix that works with geos 3 as seen in the current development tree.

This is a blocker issue. This needs to build against development as part of the
review.

-jef
Comment 13 Tim Niemueller 2008-06-29 18:33:01 EDT
Thanks for all the comments. I'm going to reply as soon as possible but we are
currently preparing hard for RoboCup in China (in two weeks) which limits time I
can put into this. I hope to get some time during the evenings next week,
otherwise after China. I'm on it, but slowed down.
Comment 14 Tim Niemueller 2008-08-05 04:08:34 EDT
I've updated the package to the latest stable release 2.1.1.

SRPM URL: http://fedorapeople.org/~timn/player/player-2.1.1-1.fc9.src.rpm

A scratch build is at http://koji.fedoraproject.org/koji/taskinfo?taskID=759170. It still fails for the same component. I'm going to setup a Rawhide VM to investigate this.

For now to get going with the package I propose to disable the GEOS support. The GIS features are nice to have, but not essential in the very beginning to get the simulator running. It can be conditionally enabled on F-9 for now, disabled on F-10. Once we have a fix or it's fixed in the Player project we can remove this conditional building.
Comment 15 Tim Niemueller 2008-08-05 10:18:55 EDT
I've added the conditional BR and it builds fine now for dist-f10 without GEOS support.

SRPM URL: http://fedorapeople.org/~timn/player/player-2.1.1-2.fc9.src.rpm
Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=759295
Comment 16 Jef Spaleta 2008-08-07 12:48:12 EDT
(In reply to comment #15)

I wonder does the newly released player 3.0.1 codebase fix the geos issue?


Should we jump to trying to get player/stage 3.0.x into the distro?

-jef
Comment 17 Jef Spaleta 2008-08-07 13:14:59 EDT
(In reply to comment #16)

no sorry that's stage that was released not player
Comment 18 Jef Spaleta 2008-08-07 15:20:23 EDT
Created attachment 313732 [details]
rpmlint output

See attachment for full list.

3  zero-length Errors in player-docs:
Do the zero-length files need to be excluded?

1 non-standard-executable-perm in player-python:
Bogus error on a python binding library not in shared library path.

19 warnings:
all the no-documentation warnings can be ignored
most of the devel-file warnings for header and c files in player-examples can be ignored

Do we want to exclude the .a archives from the player-example payloads?
player-examples.i386: W: devel-file-in-non-devel-package /usr/lib/player/examples/plugins/exampledriver/libexampledriver.a
player-examples.i386: W: devel-file-in-non-devel-package /usr/lib/player/examples/plugins/multidriver/libmultidriver.a
player-examples.i386: W: devel-file-in-non-devel-package /usr/lib/player/examples/plugins/opaquedriver/libopaquedriver.a
player-examples.i386: W: devel-file-in-non-devel-package /usr/lib/player/examples/plugins/exampleinterface/libeginterf.a
player-examples.i386: W: devel-file-in-non-devel-package /usr/lib/player/examples/plugins/exampleinterface/libeginterfdriver.a
Comment 19 Jef Spaleta 2008-08-07 15:55:39 EDT
Formal Review Summary:

Summary:
Only one solid blocker
License text is incorrect.
License: GPLv2+ and LGPLv2+
Reading the source comments they use the "or later version" clause hence the "+"

The rpmlint errors concerning the zero-length files in the docs subpackage need to be discussed however before you open up the F9 branch.  I'm marking this as approved for rawhide as long as you correct the license text.

Full Review:
naming: good
spec-naming: good
license: wrong should be GPLv2+ and LGPLv2+
md5sum matches upstream url
74d221bebd7f68a8ef258aadf0ac185b  included player-2.1.1.tar.bz2
74d221bebd7f68a8ef258aadf0ac185b  http://mesh.dl.sourceforge.net/sourceforge/playerstage/player-2.1.1.tar.bz2
passes development scratch build on koji
builds locally via mock for rawhide and f9
no locale support needed
ldconfig called post and postun
not relocatable
no dupes in files
macro usage fine
permissible code/content
docs package looks good
static package looks good
devel package looks good, requires pkgconfig as it should
docs section in main package looks good
no .la files installed
install section looks good
desktop files look good
directory ownership looks good
Comment 20 Jef Spaleta 2008-08-07 15:58:21 EDT
Created attachment 313743 [details]
Patch to spec file with review changes

Okay here's the little patch of the last round of review changes
license tag correction and removal of the unneeded real-version macro

The license tag change is the only one that is blocking.
Comment 21 Tim Niemueller 2008-08-11 10:57:34 EDT
New SRPM: http://fedorapeople.org/~timn/player/player-2.1.1-3.fc9.src.rpm

Contains your patch, Jeff. I modified the changelog date to the date when you posted that patch (was June in the patch). Is that correct/ok?

To comment #18: The zero-byte files are autogenerated by doxygen. I'd like to keep them because at some point they might contain useful content and then we might not see it.

The examples package could pack all the binaries to /usr/bin/player-example-* and the source as %doc. Would that be what you had in mind? I think we can do this also on an approved package before we push an update...
Comment 22 Jef Spaleta 2008-08-11 12:37:37 EDT
(In reply to comment #21)

Err, I'm not sure I meant for the examples to be dumped into /usr/bin/
I'm not sure that is okay or not, are any of them guis? If they are they'd need desktop files.  All the stuff I've done with examples so far have been example scripts.. not executables..so I don't have any personal experience with guidance on the matter.

Leave them where they are for now. And I'll try to get some feedback as to whether they should go into /usr/bin/ as normal executables.

-jef
Comment 23 Tim Niemueller 2008-08-11 17:08:22 EDT
Ok, we leave it as it is. But let's get this rolling and into Fedora now so that we can get going.

The desktop files for the examples do not make much sense, the example demos are meant for programmers who use Player, not for regular users. Most stuff is pretty console centric anyway. The playerv GUI for example can only be connected to a different host on the console and not via the GUI.
Comment 24 Jef Spaleta 2008-08-11 18:52:49 EDT
(In reply to comment #23)

So build it already!  Its marked as a approved.

As soon as this is available in rawhide.. I can do the stage review.

-jef
Comment 25 Tim Niemueller 2008-08-12 03:25:27 EDT
(In reply to comment #24)
> (In reply to comment #23)
> 
> So build it already!  Its marked as a approved.

Ah, I must have missed it. When did you set that? I couldn't find it in any bugzilla mail... Will do CVS request now.
Comment 26 Tim Niemueller 2008-08-12 03:27:43 EDT
New Package CVS Request
=======================
Package Name: player
Short Description: Cross-platform robot device interface and server
Owners: timn
Branches: F-8 F-9
InitialCC: makghosh
Cvsextras Commits: yes
Comment 27 Kevin Fenzi 2008-08-12 13:13:52 EDT
cvs done.
Comment 28 Tim Niemueller 2008-09-03 07:16:01 EDT
Updates have been pushed that can be used. Another update will probably move the example applications to more proper places.