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
I'm working on the review today. Sorry for the delay. -jef
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
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.
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
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
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.
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
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.
(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.
(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
(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
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
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.
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.
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
(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
(In reply to comment #16) no sorry that's stage that was released not player
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
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
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.
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...
(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
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.
(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
(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.
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
cvs done.
Updates have been pushed that can be used. Another update will probably move the example applications to more proper places.