Bug 770740 - Review Request: morse - Simulates robots using the Blender Game Engine [NEEDINFO]
Review Request: morse - Simulates robots using the Blender Game Engine
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Raphael Groner
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2011-12-28 15:04 EST by Spencer Jackson
Modified: 2015-07-21 08:26 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-01-26 11:39:57 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
tibbs: needinfo? (spencerandrewjackson)
projects.rg: needinfo? (spencerandrewjackson)
projects.rg: needinfo? (spencerandrewjackson)


Attachments (Terms of Use)

  None (edit)
Description Spencer Jackson 2011-12-28 15:04:55 EST
Spec URL: https://dl-web.dropbox.com/get/Public/morse.spec?w=5c5ce0b5
SRPM URL: http://dl.dropbox.com/u/4517312/morse-0.4.1-0.fc16.src.rpm
Description:

Hi!

I've been using Morse for a university project team, and thought I'd package it up for Fedora. A review would be much appreciated, especially as I had to modify some of the installation paths. I sent the changes I made to Morse's upstream.

Morse is a 3D robotics simulator, using the Blender Game Engine. A roboticist may create a 3D model, then attach actuators and sensors. Morse then allows them to control the model and interact with the simulated environment. A variety of middleware protocols are supported, but many of these don't seem to be packaged yet for Fedora.
Comment 1 Spencer Jackson 2011-12-28 15:06:29 EST
I forgot to mention, this is my first package, and I believe I need a sponsor... Any help in that regard would be much appreciated.
Comment 2 Spencer Jackson 2011-12-30 15:26:23 EST
New spec file.
Spec URL: http://dl.dropbox.com/u/4517312/morse.spec
SRPM URL: http://dl.dropbox.com/u/4517312/morse-0.4.1-1.fc16.src.rpm

Builds and installs documentation, correctly forces the use of python3 while bytecode compiling, as well as miscellaneous other cleanups.

rpmlint output:

[makerpm@khezef rpmbuild]$ rpmlint SPECS/morse.spec /var/lib/mock/fedora-16-x86_64/result/morse-* morse morse-debuginfo
SPECS/morse.spec: W: invalid-url Source0: laas-morse-0.4.1-0-g15dc857.tar.gz
morse.src: W: spelling-error %description -l en_US middleware -> middle ware, middle-ware, middleweight
morse.src: W: invalid-url Source0: laas-morse-0.4.1-0-g15dc857.tar.gz
morse.x86_64: W: no-manual-page-for-binary morseexec
morse.x86_64: W: no-manual-page-for-binary morse
morse.x86_64: W: no-manual-page-for-binary morseexec
morse.x86_64: W: no-manual-page-for-binary morse
5 packages and 1 specfiles checked; 0 errors, 7 warnings.
Comment 3 Adrien Devresse 2012-01-06 11:25:57 EST
Hi, 

Quick comments about your spec file :

- use the %{_isa} macro for your "Requires" dependencies on binary packages.

- "-DCMAKE_INSTALL_PREFIX=/usr" is useless, it is already included in the %cmake macro

- %{_libdir} could be used instead of /usr/lib64/ with your cmake command

- same with %{_includedir} instead of /usr/include
Comment 4 Spencer Jackson 2012-01-08 17:32:16 EST
Thanks, adev! I've applied your recommendations to the spec.

New spec URL: http://dl.dropbox.com/u/4517312/morse.spec
New SRPM URL: http://dl.dropbox.com/u/4517312/morse-0.4.1-2.fc16.src.rpm

rpmlint morse morse-debuginfo RPMS/x86_64/morse-0.4.1-2.fc16.x86_64.rpm RPMS/x86_64/morse-debuginfo-0.4.1-2.fc16.x86_64.rpm SRPMS/morse-0.4.1-2.fc16.src.rpm SPECS/morse.spec 
morse.x86_64: W: no-manual-page-for-binary morseexec
morse.x86_64: W: no-manual-page-for-binary morse
morse.x86_64: W: no-manual-page-for-binary morseexec
morse.x86_64: W: no-manual-page-for-binary morse
morse.src: W: spelling-error %description -l en_US middleware -> middle ware, middle-ware, middleweight
morse.src: W: invalid-url Source0: laas-morse-0.4.1-0-g15dc857.tar.gz
SPECS/morse.spec: W: invalid-url Source0: laas-morse-0.4.1-0-g15dc857.tar.gz
5 packages and 1 specfiles checked; 0 errors, 7 warnings.

I just sent an email to the packaging list, ( http://lists.fedoraproject.org/pipermail/packaging/2012-January/008067.html ) basically asking what to do about the middleware protocols which are supported by Morse, but not packaged in Fedora. Some of these, like ROS, might take a while to be supported. Any thoughts on how to handle that would be appreciated.

Upstream also recommended that I create subpackages for each middleware. That makes a lot of sense, but I should probably figure out what's the best way to handle the unsupported middlewares before I do that...
Comment 5 Adrien Devresse 2012-01-09 05:49:39 EST
Hi, This is an informal review, I am not a sponsor.


[PASS] MUST: rpmlint must be run on the source rpm and all binary rpms the build produces. The output should be posted in the review.

morse.src: I: enchant-dictionary-not-found en_US
morse.src: W: invalid-url Source0: laas-morse-0.4.1-0-g15dc857.tar.gz
morse.x86_64: W: no-manual-page-for-binary morseexec
morse.x86_64: W: no-manual-page-for-binary morse
3 packages and 0 specfiles checked; 0 errors, 4 warnings.

 -> no man pages

[PASS] MUST: The package must be named according to the Package Naming Guidelines .
[PASS] MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption.
[PASS] MUST: The package must meet the Packaging Guidelines.


[PASS] MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
[PASS] MUST: The License field in the package spec file must match the actual license. 
[PASS] MUST: 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 must be included in %doc.
[PASS] MUST: The spec file must be written in American English. 
[PASS] MUST: The spec file for the package MUST be legible. 
[PASS] MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.

rpmdev-md5 morse-0.4.1-2.fc16.src.rpm 
293849e6e24b0b3adf2dbd9ae0064300  morse-0.4.1-2.fc16.src.rpm
0e9c30808c82062d284a528c052e16f2  morse.desktop
794f77239d535d61861426d450b66850  morse-0.4.1-lsbpathfix.patch
a2b9c70415653665ad5d90504ffebc5e  morse-0.4.1.usepythonsitearch.patch
b57e3f3dd1eed67ce201ddfa14b920ad  laas-morse-0.4.1-0-g15dc857.tar.gz
5ced6d1eec3f7a66e6f0e41bc64de92d  morse.spec

md5sum 0.4.1 
b57e3f3dd1eed67ce201ddfa14b920ad  0.4.1

[PASS] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
	-> see koji
[PASS] MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line. 
[PASS] MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.
[PASS] MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
[PASS] MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. 
[PASS] MUST: Packages must NOT bundle copies of system libraries.
[PASS] MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. 
[PASS] MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. 
[PASS] MUST: A Fedora package must not list a file more than once in the spec file's %files listings. (Notable exception: license texts in specific situations)
[PASS] MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. 
[PASS] MUST: Each package must consistently use macros. 
[PASS] MUST: The package must contain code, or permissable content. 
[PASS] MUST: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). 
[PASS] MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. 
[PASS] MUST: Header files must be in a -devel package. 
[PASS] MUST: Static libraries must be in a -static package. 
[PASS] MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. 
[PASS] MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name}%{?_isa} = %{version}-%{release} 
[PASS] MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.
[PASS] MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. 
[PASS] MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. 
[PASS] MUST: All filenames in rpm packages must be valid UTF-8. 


[PASS] SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. 
[PASS] SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. 
[PASS] SHOULD: The reviewer should test that the package builds in mock. 
[PASS] SHOULD: The package should compile and build into binary rpms on all supported architectures. 

koji build --arch-override=x86_64 --scratch f16-candidate morse-0.4.1-2.fc16.src.rpm 
Uploading srpm: morse-0.4.1-2.fc16.src.rpm
[====================================] 100% 00:05:49  75.06 MiB 219.88 KiB/sec
Created task: 3632889
Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=3632889
Watching tasks (this may be safely interrupted)...
3632889 build (f16-candidate, morse-0.4.1-2.fc16.src.rpm): free
3632889 build (f16-candidate, morse-0.4.1-2.fc16.src.rpm): free -> open (ppc12.phx2.fedoraproject.org)
  3632890 buildArch (morse-0.4.1-2.fc16.src.rpm, x86_64): open (x86-03.phx2.fedoraproject.org)
  3632890 buildArch (morse-0.4.1-2.fc16.src.rpm, x86_64): open (x86-03.phx2.fedoraproject.org) -> closed
  0 free  1 open  1 done  0 failed
3632889 build (f16-candidate, morse-0.4.1-2.fc16.src.rpm): open (ppc12.phx2.fedoraproject.org) -> closed
  0 free  0 open  2 done  0 failed

koji build --arch-override=i386 --scratch f16-candidate morse-0.4.1-2.fc16.src.rpm 
Uploading srpm: morse-0.4.1-2.fc16.src.rpm
[====================================] 100% 00:05:52  75.06 MiB 217.76 KiB/sec
Created task: 3632894
Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=3632894
Watching tasks (this may be safely interrupted)...
3632894 build (f16-candidate, morse-0.4.1-2.fc16.src.rpm): open (x86-16.phx2.fedoraproject.org)
  3632895 buildArch (morse-0.4.1-2.fc16.src.rpm, i386): open (x86-13.phx2.fedoraproject.org)
  3632895 buildArch (morse-0.4.1-2.fc16.src.rpm, i386): open (x86-13.phx2.fedoraproject.org) -> closed
  0 free  1 open  1 done  0 failed
3632894 build (f16-candidate, morse-0.4.1-2.fc16.src.rpm): open (x86-16.phx2.fedoraproject.org) -> closed
  0 free  0 open  2 done  0 failed

3632894 build (f16-candidate, morse-0.4.1-2.fc16.src.rpm) completed successfully


[PASS] SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example.
[PASS] SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. 
[PASS] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. 
[PASS] SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb. 
[PASS] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. 
[FAIL] SHOULD: your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense.

	-> add man pages
Comment 6 Spencer Jackson 2012-01-12 15:35:43 EST
Just uploaded a new spec
Spec URL: http://dl.dropbox.com/u/4517312/morse.spec
SRPM URL: http://dl.dropbox.com/u/4517312/morse-0.4.1-3.fc16.src.rpm

I've added the missing man pages, as well as properly generate documentation which is placed into a doc subpackage. Further, I've added optional build time dependencies on features which have unpackaged dependencies.

[makerpm@khezef rpmbuild]$ rpmlint morse morse-debuginfo morse-doc SPECS/morse.spec /var/lib/mock/fedora-16-x86_64/result/morse-0.4.1-3.fc16.src.rpm 
morse.x86_64: W: spelling-error %description -l en_US ros -> rps, rod, rs
morse.x86_64: W: spelling-error %description -l en_US Middleware -> Middle ware, Middle-ware, Middleweight
morse.x86_64: W: spelling-error %description -l en_US subpackages -> sub packages, sub-packages, prepackages
SPECS/morse.spec: W: invalid-url Source0: laas-morse-0.4.1-0-g15dc857.tar.gz
morse.src: W: spelling-error %description -l en_US middleware -> middle ware, middle-ware, middleweight
morse.src: W: spelling-error %description -l en_US ros -> rps, rod, rs
morse.src: W: spelling-error %description -l en_US Middleware -> Middle ware, Middle-ware, Middleweight
morse.src: W: spelling-error %description -l en_US subpackages -> sub packages, sub-packages, prepackages
morse.src: W: invalid-url Source0: laas-morse-0.4.1-0-g15dc857.tar.gz
4 packages and 1 specfiles checked; 0 errors, 9 warnings.
Comment 7 Spencer Jackson 2012-01-17 16:13:38 EST
New spec
Spec URL: http://dl.dropbox.com/u/4517312/morse.spec
SRPM URL: http://dl.dropbox.com/u/4517312/morse-0.4.1-4.fc16.src.rpm

[makerpm@khezef rpmbuild]$ rpmlint /var/lib/mock/fedora-16-x86_64/result/morse-* morse morse-doc morse-debuginfo SPECS/morse.spec
morse.src: W: spelling-error %description -l en_US middleware -> middle ware, middle-ware, middleweight
morse.src: W: invalid-url Source0: laas-morse-0.4.1-0-g15dc857.tar.gz
SPECS/morse.spec: W: invalid-url Source0: laas-morse-0.4.1-0-g15dc857.tar.gz
7 packages and 1 specfiles checked; 0 errors, 3 warnings.
Comment 8 Pierre-YvesChibon 2012-08-22 04:37:57 EDT
@Adrien, now that you are an official packager, what about finishing this review ?
Comment 9 Jason Tibbitts 2013-05-01 18:40:28 EDT
I am triaging old review tickets.  I can't promise a review if you reply, but by closing out the stale tickets we can devote extra attention to the ones which aren't stale.

This fails to build for me; here's a koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5322961

In addition, we do have guidelines for dealing with github which you might want to look over: http://fedoraproject.org/wiki/Packaging:SourceURL
Comment 10 Raphael Groner 2014-10-05 10:09:15 EDT
Any news here? I could take the review process if you may like.
Comment 11 Raphael Groner 2014-10-28 19:33:04 EDT
Your spec file is not ready for a formal review. Please fix following listed points. After done that, I could have a deeper look.

> # Tarballs can be obtained from GitHub at https://github.com/laas/morse/tarball/0.4.1
> Source0:	laas-morse-0.4.1-0-g15dc857.tar.gz
%global name    morse
%global commit  g15dc857
%global version 0.4.1
Source0:	https://github.com/laas/%{name}/tarball/%{version}#/laas-name-%{version}-0-%{commit}.tar.gz

Analogously, you should give the individual URL directly into PatchX.


> %{?with_pocolibs:
What's that? I would not suggest conditional packaging. What is the purpose?

> Requires:	%{name}%{?_isa} = %{version}-%{release}
Remove %{?_isa} completely from Requires. Otherwise, you won't be able to build cross-arch.

> BuildRequires:	swig
> BuildRequires:	pocolibs-devel
Move those on top to the BR for the main package. There's only one %build allowed for all sub-packages.

> %build:
You should use %cmake macro instead of direct cmake call with tons of standard parameters.
https://fedoraproject.org/wiki/Packaging:Cmake?rd=Packaging/cmake

> pushd doc
> make 
> popd
cd doc ; make


> %install
> desktop-file-install --vendor="fedora" \
The Vendor tag should not be used. It is set automatically by the build system.
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Tags

> # Move …
Use install -d instead of mkdir -p to preserve timestamps etc.
Comment 13 Raphael Groner 2015-01-12 05:48:39 EST
ping? Are you still interested in maintaining this package?

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