Bug 519071

Summary: Review Request: wiipresent - Giving presentations (or control applications) with your Wiimote
Product: [Fedora] Fedora Reporter: Jens Kuehnel <bugzilla-redhat>
Component: Package ReviewAssignee: Dominic Hopf <dmaphy>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: cwickert, dmaphy, fedora-package-review, michael.monreal+bugs, msuchy, notting
Target Milestone: ---Flags: msuchy: fedora‑review-
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-02-19 05:55:19 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 201449    

Description Jens Kuehnel 2009-08-24 17:13:08 EDT
Spec URL: http://www.kuehnel.org/wiipresent.spec
SRPM URL: http://www.kuehnel.org/wiipresent-0.7.5.2-2.fc11.src.rpm
Description:
wiipresent is a program to control applications using your Nintendo Wii
Remote, short wiimote. It was originally developed for giving presentations
with your wiimote, but can also be used to control your mouse-pointer and
control various applications.
Comment 1 Dominic Hopf 2009-08-28 14:33:17 EDT
$ rpmlint wiipresent.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint wiipresent-0.7.5.2-2.fc11.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint wiipresent-0.7.5.2-2.fc11.i586.rpm  wiipresent-debuginfo-0.7.5.2-2.fc11.i586.rpm
2 packages and 0 specfiles checked; 0 errors, 0 warnings.


MUSTs
-----

OK: packaged is named according to the package naming guidelines
OK: specfile name matches %{name}.spec
OK: package seems to meet packaging guidelines
OK: license in specfile matches actual license and meets licensing guidelines
OK: license file is included in %doc
OK: specfile is written in AE
OK: specfile is legible
OK: sourcefile in the package is the same as provided in the mentioned source,
    md5sum fits
OK: package compiles successfully
OK: all build dependencies are listed in BuildRequires
N/A: package handles locales properly
     there are no locales installed with this package
N/A: call ldconfig in %post and %postun
     there is no binary installed with this package
OK: package is not designed to be relocatable
OK: package owns directorys it creates
OK: does not list a file more than once in the %files listing
OK: %files section includes %defattr and permissions are set properly
OK: %clean section is there and contains rm -rf %{buildroot}
NOT OK: macros are consistently used
OK: package contains code
N/A: subpackage for large documentation files
     there are no large documentation files
OK: program runs properly without files listed in %doc
N/A: header files are in a -devel package
     there are no header files
N/A: static libraries are in a -static package
     there are no static libs
N/A: require pkgconfig if package contains a pkgconfig(.pc)
     there is no pkgconfig file
N/A: put .so-files into -devel package if there are library files with suffix
     there is no library with suffix, in fact there isn't any library
N/A: devel package includes fully versioned dependency for the base package
     there is no devel package
N/A: any libtool archives are removed
     there are no libtool archives
OK: contains desktop file if it is a GUI application
OK: package does not own any files or directories owned by other packages
OK: buildroot is removed at beginning of %install
N/A: filenames are encoded in UTF-8
     not necessary since there are no non-ASCII filenames


SHOULD
------
N/A: non-English translations for description and summary
     there are no other languages supported by this package, in fact it does not
     provide any localization. I assume localizations are not needed for this
     package.
OK: package builds in mock
OK: package builds into binary rpms for all supported architectures
N/A: program runs
     I did not test myself if the program works as it should
N/A: subpackages contain fully versioned dependency for the base package
     there are no subpackages
N/A: pkgconfig file is placed in a devel package
     there is no pkgconfig file
N/A: require package providing a file instead of the file itself
     no files outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin are required

Please do not use macros for rm and make install in the %install section. This
also should fix the consistency of using macros which is not ok.
Comment 2 Christoph Wickert 2009-08-28 17:45:28 EDT
Pleas note that 

%post
/usr/bin/update-desktop-database -q || :

%postun
/usr/bin/update-desktop-database -q || :

is not necessary, because the desktop file contains no mimetype. This is only needed it an application is supposed to open a certain mime type, so the "Open with..." entry gets created.

And of course,

Requires(post): desktop-file-utils
Requires(postun): desktop-file-utils

are not necessary ether. This would only be neccessary if there was a mime type AND you want to build this package on EPEL/Fedora < 5, see
https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#desktop-database


I don't like the summary. Summary should be short an precise but not whole sentences. How about: "Tool to control applications or presentations with a Wiimote"?


I didn't test it, but 

%if 0%{?fedora} <= 11
BuildRequires: xorg-x11-proto-devel
%else
BuildRequires: libXi-devel
%endif

looks bogus to me, as libXi-devel requires xorg-x11-proto-devel.


%{_mandir}/man1/wiipresent.1* does not need to be tagged as %doc, rpmbuild will take care of this. It doesn't do no harm, it's just a hint.


Finally: the build root tag should be

%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

or even better

%(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)

Doesn't really matter ether, but IMO for new packages we should follow the guidelines from https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

That's all from my side, nothing more to add.
Comment 3 Christoph Wickert 2009-08-28 18:39:00 EDT
One more thing....

The desktop file contains:

Categories=Application;Office;X-Red-Hat-Base;
...
GenericName=Written by Dag Wieers and Geerd-Dietger Hoffmann

"Application" no longer is a valid category, see
http://standards.freedesktop.org/menu-spec/latest/apa.html
"X-Red-Hat-Base" is obsolete, too.
The last one is an abuse of the GenericName tag that doesn't make any sense. You could do this with desktop-file-install (instead of desktop-file-validate):

desktop-file-install                                    \
--remove-category="Application"                         \
--remove-category="X-Red-Hat-Base"                      \
--copy-name-to-generic-name                             \
--delete-original                                       \
--dir=%{buildroot}%{_datadir}/applications              \
%{buildroot}/%{_datadir}/applications/wiipresent.desktop

Instead of --copy-name-to-generic-name you can also use --remove-key=GenericName.
Comment 4 Jens Kuehnel 2009-08-28 18:51:44 EDT
Spec URL: http://www.kuehnel.org/wiipresent.spec
SRPM URL: http://www.kuehnel.org/wiipresent-0.7.5.2-3.fc11.src.rpm

Fixed all marked topics in 0.7.5.2-3
Comment 5 Dominic Hopf 2009-08-29 06:34:56 EDT
The Links unfortunately do not work. "Forbidden" for the Spec-File and "Not Found" for the SRPM.
Comment 6 Jens Kuehnel 2009-08-31 08:31:44 EDT
Ups, sometimes I hate SELinux.

Fixed now.

Sorry
Comment 7 Dominic Hopf 2009-08-31 19:02:02 EDT
No Problem, I was able now to have another look on your package. :)

$ rpmlint wiipresent.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint wiipresent-0.7.5.2-3.fc11.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint wiipresent-0.7.5.2-3.fc11.x86_64.rpm wiipresent-debuginfo-0.7.5.2-3.fc11.x86_64.rpm
wiipresent.x86_64: W: incoherent-version-in-changelog 0.7.5.2-2 ['0.7.5.2-3.fc11', '0.7.5.2-3']

NOTE: You forgot to update your %changelog.


MUSTs
-----

OK: packaged is named according to the package naming guidelines
OK: specfile name matches %{name}.spec
OK: package seems to meet packaging guidelines
OK: license in specfile matches actual license and meets licensing guidelines
OK: license file is included in %doc
OK: specfile is written in AE
OK: specfile is legible
OK: sourcefile in the package is the same as provided in the mentioned source,
    md5sum fits
OK: package compiles successfully
OK: all build dependencies are listed in BuildRequires
N/A: package handles locales properly
     there are no locales installed with this package
N/A: call ldconfig in %post and %postun
     there is no binary installed with this package
OK: package is not designed to be relocatable
OK: package owns directorys it creates
OK: does not list a file more than once in the %files listing
OK: %files section includes %defattr and permissions are set properly
OK: %clean section is there and contains rm -rf %{buildroot}
OK: macros are consistently used
OK: package contains code
N/A: subpackage for large documentation files
     there are no large documentation files
OK: program runs properly without files listed in %doc
N/A: header files are in a -devel package
     there are no header files
N/A: static libraries are in a -static package
     there are no static libs
N/A: require pkgconfig if package contains a pkgconfig(.pc)
     there is no pkgconfig file
N/A: put .so-files into -devel package if there are library files with suffix
     there is no library with suffix, in fact there isn't any library
N/A: devel package includes fully versioned dependency for the base package
     there is no devel package
N/A: any libtool archives are removed
     there are no libtool archives
OK: contains desktop file if it is a GUI application
OK: package does not own any files or directories owned by other packages
OK: buildroot is removed at beginning of %install
N/A: filenames are encoded in UTF-8
     not necessary since there are no non-ASCII filenames


SHOULD
------
N/A: non-English translations for description and summary
     there are no other languages supported by this package, in fact it does not
     provide any localization. I assume localizations are not needed for this
     package.
OK: package builds in mock
OK: package builds into binary rpms for all supported architectures
N/A: program runs
     I did not test myself if the program works as it should
N/A: subpackages contain fully versioned dependency for the base package
     there are no subpackages
N/A: pkgconfig file is placed in a devel package
     there is no pkgconfig file
N/A: require package providing a file instead of the file itself
     no files outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin are required


BLOCKERS:
 - As written above, you forgot to update your %changelog, i'd like to see this
   added before approving

Just as hints:
 - Indenting the arguments for desktop-file-install would make
   the spec i bit more legible imho. It's on your's if you like to do so.
 - I don't see why you are using a wildcard for the manpage, I think
   writing 'wiipresent.1.gz' would do the job also. If there is no specific
   reason for that I would recommend you to fix that with your next release.
Comment 8 Michael Monreal 2009-12-05 07:36:27 EST
It has been some month... seeing if this is just blocking on the changelog, is there hope we will see this package included soon?
Comment 9 Miroslav Suchý 2012-12-11 17:54:02 EST
Ping? Any progress here? Or we can close this review?
Comment 10 Miroslav Suchý 2013-02-19 05:55:19 EST
Stalled Review. Closing per:
https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
If you ever want to continue with this review, please reopen or
submit new review.