Bug 519071 - Review Request: wiipresent - Giving presentations (or control applications) with your Wiimote
Review Request: wiipresent - Giving presentations (or control applications) w...
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dominic Hopf
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2009-08-24 17:13 EDT by Jens Kuehnel
Modified: 2013-02-19 05:55 EST (History)
6 users (show)

See Also:
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: ---
msuchy: fedora‑review-


Attachments (Terms of Use)

  None (edit)
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.

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