Bug 441072 - Review Request: cwiid - Library and tools for comunicating with a wiimote
Review Request: cwiid - Library and tools for comunicating with a wiimote
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-05 15:34 EDT by Victor Bogado
Modified: 2008-04-30 14:03 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-04-29 02:31:54 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
proposal spec file (4.37 KB, text/plain)
2008-04-30 13:33 EDT, Mamoru TASAKA
no flags Details

  None (edit)
Description Victor Bogado 2008-04-05 15:34:37 EDT
Spec URL: http://bogado.net/rpm/cwiid.spec
SRPM URL: http://bogado.net/rpm/cwiid-0.6.00-1bog.src.rpm
Description: This package has a library and some tools that allows communication with a wiimote using a bluetooth connection. The package creates four different rpm files:

 - cwiid : base library.
 - cwiid-devel : development files.
 - cwiid-python2 : python bindings.
 - cwiid-wmgui : test program with a gtk gui.
 - cwiid-wminput : Program that allows user to control mouse/keyboard with the wiimote (this package is affected by the bug #401951 '"/dev/input/uinput" without user rights (0600 root root)').
Comment 1 Victor Bogado 2008-04-05 15:36:13 EDT
Ps. homepage of the packaged software: http://abstrakraft.org/cwiid/
Comment 2 Jason Tibbitts 2008-04-08 17:35:54 EDT
I don't see you in the account system; is this your first package for Fedora?
Comment 3 josh.kayse 2008-04-08 19:10:33 EDT
I started to do a pre-review but the package wouldn't build as is.

You need to start by adding python-devel >= 2.4 to the BuildRequires

Doing that produces the following error:

RPM build errors:
    Installed (but unpackaged) file(s) found:
   /usr/lib64/python2.5/site-packages/cwiid-0.6.00-py2.5.egg-info

Take care of those 2 things and I'll do another pre-review.
Comment 4 josh.kayse 2008-04-08 19:11:26 EDT
Also, you need something under changelog and your package is not rpmlint clean:

cwiid.src: E: description-line-too-long Cwiid is a library that enables your
application to communicate with a wiimote using a bluetooth connection.
cwiid.src: E: no-changelogname-tag
Comment 5 josh.kayse 2008-04-08 19:40:54 EDT
Ok, I built the packages by adding 
%{python_sitelib}/%{name}-%{version}-py2.5.egg-info to the python2 %files
section (I do not know if this is the correct thing to do or not, I don't know
what that file is used for)

For testing purposes I took care of the easy things for rpmlint (description too
long and summary ending in . and no changelog) which produced the following
output on the built packages:

cwiid.x86_64: W: no-documentation
cwiid-devel.x86_64: W: no-documentation
cwiid-devel.x86_64: W: spurious-executable-perm /usr/lib64/libcwiid.a
cwiid-devel.x86_64: W: spurious-executable-perm /usr/include/cwiid.h
cwiid-python2.x86_64: W: no-documentation
cwiid-python2.x86_64: E: non-standard-executable-perm
/usr/lib64/python2.5/site-packages/cwiid.so 0775
cwiid-wmgui.x86_64: W: summary-not-capitalized wiimote connectio test application
cwiid-wminput.x86_64: E: script-without-shebang /etc/cwiid/wminput/buttons
cwiid-wminput.x86_64: E: script-without-shebang /etc/cwiid/wminput/gamepad
cwiid-wminput.x86_64: E: script-without-shebang /etc/cwiid/wminput/neverball
cwiid-wminput.x86_64: E: script-without-shebang /etc/cwiid/wminput/nunchuk_acc_ptr
cwiid-wminput.x86_64: E: script-without-shebang /etc/cwiid/wminput/ir_ptr
cwiid-wminput.x86_64: E: script-without-shebang /etc/cwiid/wminput/acc_ptr

Remaining packaging guidelines:
MUST:
+ rpmlint output
  see above
+ package name satisfies the packaging naming guidelines
+ specfile name matches the package base name
- package should satisfy packaging guidelines
  not rpmlint clean
+ license meets guidelines and is acceptable to Fedora
+ license matches actual package license
- includes LICENSE file in %doc
  does not include LICENSE file
+ spec file written in American English
+ spec file is legible
+ upstream sources match sources in the srpm
- package successfully builds on at least one architecture
  Does not build with sources provided.  Builds on x86_64 once initial
recommendations are met
+ ExcludeArch bugs filed
- BuildRequires list all build dependencies
  needs python-devel >= 2.4
? %find_lang instead of %{_datadir}/locale/*
+ binary RPM with shared library files must call ldconfig in %post and %postun
+ does not use Prefix: /usr
+ package owns all directories it creates
+ no duplicate files in %files
+ %defattr line
+ %clean contains rm -rf $RPM_BUILD_ROOT
+ consistent use of macros
+ package must contain code or permissible content
+ large documentation files should go in -doc subpackage
+ files marked %doc should not affect package
+ header files should be in -devel
+ static libraries should be in -static
+ packages containing pkgconfig (.pc) files need 'Requires: pkgconfig'
+ libfoo.so must go in -devel
+ -devel must require the fully versioned base
+ packages should not contain libtool .la files
+ packages containing GUI apps must include %{name}.desktop file
+ packages must not own files or directories owned by other packages

Optional:

+ if there is no license file, packager should query upstream
+ translations of description and summary for non-English languages, if available
+ builds in mock
+ builds on i386 and x86_64
? review should test the package functions as described
+ scriptlets should be sane
+ pkgconfig files should go in -devel
+ shouldn't have file dependencies outside /etc /bin /sbin /usr/bin or /usr/sbin

Take care of those things and I'll do another pre-review.
Comment 6 Victor Bogado 2008-04-09 09:24:39 EDT
Yes it is my first try at packaging something for fedora, so please go easy on
me (hehee). ':-) Should I create an account? 

The main problem I have with your direction is that the file you said
'/usr/lib64/python2.5/site-packages/cwiid-0.6.00-py2.5.egg-info' is not
installed in my version(*). If add this file the rpm fails to build in my
machine. What I did was to change the line that specify
"%{python_sitelib}/cwiid.so" to "
%{python_sitelib}/*" since this file is on that same dir this may cover both
build environments.

Also I am checking the rpmlint errors, I hope I can upload a new version soon,
thanks.

(*)I am building it with the fedora 8, should I be building it with a rawride
version? I usually prefer to use the stable version, but I guess it should no be
hard to make a building VM to do this.

Comment 7 Jason Tibbitts 2008-04-09 16:08:06 EDT
The reason I ask is that we have specific procedures for new packagers.  Please
look over the documentation at
http://fedoraproject.org/wiki/PackageMaintainers/Join.

You're well on your way, but be aware that you will need a sponsor so please do
provide that extra information which is requested.
Comment 8 Victor Bogado 2008-04-09 18:44:36 EDT
There I fixed almost every rpmlint problem and tried to fix the problem that
appeared in josh.kayse@gtri.gatech.edu compilation. 

But there are still some problems left in rpmlint, but I don't know how to solve
them : 

cwiid.x86_64: W: incoherent-version-in-changelog 0.6.00-2 0.6.00-2bog

I use a different distribution tag to make my rpms, so I can find them easily
among the installed packages. I don't want to write the name of the distribution
in the change log because it will cause the same problem when compiled for other
distributions. 

cwiid-devel.x86_64: W: no-documentation
cwiid-python2.x86_64: W: no-documentation

All the documentation went to the right places, those two packages simply do not
have any real documentation. 

The spec file was overwritten, but the corresponding src.rpm is now : 
http://bogado.net/rpm/cwiid-0.6.00-2bog.src.rpm
Comment 9 Victor Bogado 2008-04-09 18:46:25 EDT
Oh one more thing, there is no LICENSE file but there is a COPYING file that is
now bundled with the main package. Would this cover the "includes LICENSE file
in %doc" point?
Comment 10 Victor Bogado 2008-04-19 12:52:29 EDT
My packages are revisioned using git at gitorious: 

http://git.gitorious.org/specs-for-fedora/mainline.git
Comment 11 Mamoru TASAKA 2008-04-19 14:12:51 EDT
Well, for 0.6.00-2:

* BuildRequies:
  - python-devel requires python so "BuildRequires: python" is
    redundant.

* Requires
  - Please check Requires for cwiid-devel package. 
    %_includedir/cwiid.h contains:
----------------------------------------------------------
    62  #include <time.h>
    63  #include <bluetooth/bluetooth.h>        /* bdaddr_t */
    64  
----------------------------------------------------------
    This means cwiid-devel must have "Requies: bluez-libs-devel"

* License:
  - The license tag is a bit complex.
    * wminput/action_enum.txt is GPLv2 (not GPLv2+)
    * And this file is used as:
----------------------------------------------------------
   198  gawk -f action_enum.awk action_enum.txt > action_enum.c
   210  gcc -g -Wall -W -DHAVE_CONFIG_H
-I/builddir/build/BUILD/cwiid-0.6.00/common/include -I../libcwiid
-DWMINPUT_CONFIG_DIR=\"/etc/cwiid/wminput\"
-DCWIID_PLUGINS_DIR=\"/usr/lib/cwiid/plugins\" -I/usr/include/python2.5   -c -o
action_enum.o action_enum.c
   227  gcc -o wminput main.o conf.o c_plugin.o uinput.o action_enum.o util.o
py_plugin.o parser.o lexer.o -L../libcwiid -rdynamic -lcwiid -ldl -lpython2.5
----------------------------------------------------------
      So %_bindir/wminput is GPLv2.

    As a result, please change the license tag of -wminput to
    "GPLv2".

* Compilation flags
  - Fedora specific compilation flags are not correctly honored.
----------------------------------------------------------
   164  make[1]: Entering directory `/builddir/build/BUILD/cwiid-0.6.00/libcwiid'
   165  gcc -g -Wall -W -DHAVE_CONFIG_H
-I/builddir/build/BUILD/cwiid-0.6.00/common/include -fpic   -c -o bluetooth.o
bluetooth.c
----------------------------------------------------------
    You can check what flags Fedora uses on compilation by
    "$ rpm --eval %optflags"

* Directory ownership issue
  - Please make it sure that the directory created when installing
    a rpm is owned by the rpm.
    For example, the directory %_libdir/cwiid is not owned by any
    package.

* static archive
  - Remove static archive unless needed.
    "Exclusion of Static Libraries" of
    http://fedoraproject.org/wiki/Packaging/Guidelines

* GUI app
  - as %{_bindir}/wmgui is GUI program, an appropriate desktop file
    must be installed. Check the section "Desktop files" of
    http://fedoraproject.org/wiki/Packaging/Guidelines

* Documents
  - Please add "AUTHORS" to %doc
  - Also adding "NEWS" is preferable.
Comment 12 Mamoru TASAKA 2008-04-19 14:14:47 EDT
When you upgrade your spec/srpm, please write the URLs of your spec/srpm
so that we can download them from the URLs directly by "wget -N", for example.
Comment 13 Victor Bogado 2008-04-19 16:00:24 EDT
Done all those points the new srpm and specs are at :

http://bogado.net/rpm/cwiid-0.6.00-3.bog9.src.rpm
http://bogado.net/rpm/cwiid.spec
Comment 14 Mamoru TASAKA 2008-04-20 12:03:45 EDT
Now koji/CVS/etc seems under construction so I cannot
check binary rpms rebuilt from your srpm, however
for 0.6.00-3:

* SourceURL:
  - I recommend to use %{name} and %{version}. With this
    you won't have to change Source0 URL when version is
    upgraded.

* BuildRequires
  - Rebuild fails because "BuildRequires: desktop-file-utils"
    is missing.

* License tag for -wminput subpackage
  - License tag "GPLv2" is okay, however would you write some
    comments in the spec file to explain why this subpackage
    has the license tag?

* Compiler flags
  - Fedora specific compiler flags are not yet correctly honored.

* Directory ownership issue/Duplicate files
  - There are many warnings like:
-------------------------------------------------------------
warning: File listed twice: /etc/cwiid/wminput
warning: File listed twice: /etc/cwiid/wminput/acc_ptr
warning: File listed twice: /etc/cwiid/wminput/acc_ptr
warning: File listed twice: /etc/cwiid/wminput/buttons
warning: File listed twice: /etc/cwiid/wminput/buttons
.....................
--------------------------------------------------------------

    ! If you write the %files entry
--------------------------------------------------------------
%files
%{_libdir}/cwiid
--------------------------------------------------------------
      this contains the directory %_libdir/cwiid and all files/
      directories/etc under the directory, while 
--------------------------------------------------------------
%files
%dir %{_libdir}/cwiid
--------------------------------------------------------------
      contains the directory %_libdir/cwiid only.

* Desktop file
  - wmgui desktop file has Icon entry, however no icons seem to
    be installed?

Well, as this is NEEDSPONSOR ticket:

-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------
Comment 15 Victor Bogado 2008-04-24 17:55:33 EDT
I made all the chages and I do have another package wating for review, and I
even made some changes on it for myself based on the points you posted here. It
is a 3d game using SDL, it does have a questionable item in it, there is a
non-free (freely distributed for non-comercial uses) font, I removed it from the
final package but since the sources used must be the same as the upstream the
font file gets to be distributed with the source rpm, not sure if this is a problem.

The bug number for the review is #443238.
Comment 16 Victor Bogado 2008-04-24 17:56:46 EDT
the latest spec and src rpm are bellow : 

http://bogado.net/rpm/cwiid-0.6.00-5.bog9.src.rpm
http://bogado.net/rpm/cwiid.spec
Comment 17 Mamoru TASAKA 2008-04-25 11:45:56 EDT
Almost okay.
For 0.6.00-5:

* Directory permission issue
  - rpmlint shows:
----------------------------------------------------------
cwiid-wminput.i386: E: non-standard-dir-perm /etc/cwiid 0644
cwiid-wminput.i386: E: non-standard-dir-perm /etc/cwiid/wminput 0644
----------------------------------------------------------
    These directories should have 0755 permission.

    When you write
----------------------------------------------------------
    97  %files wminput
    98  %config(noreplace) %attr(0644,root,root) %{_sysconfdir}/cwiid
----------------------------------------------------------
    this actually sets all permissions of files/directories/etc
    under %_sysconfdir/cwiid ant itself to 0644.

    To avoid this, I recommend to write just:
----------------------------------------------------------
%config(noreplace) %{_sysconfdir}/cwiid/
----------------------------------------------------------
    (I usually add slash to the end intentionally if it is a
     directory) and change the permissions beforehand at %install like:
----------------------------------------------------------
find $RPM_BUILD_ROOT%{_sysconfdir} -type d -exec chmod 0644 {} ';'
----------------------------------------------------------
Comment 18 Victor Bogado 2008-04-25 13:43:57 EDT
should it be: 
------------------------------------------------------------------
find $RPM_BUILD_ROOT%{_sysconfdir} -type d -exec chmod 0755 {} ';'
------------------------------------------------------------------

?
Comment 19 Mamoru TASAKA 2008-04-25 13:55:15 EDT
It is
----------------------------------------------------------
find $RPM_BUILD_ROOT%{_sysconfdir} -type f -exec chmod 0644 {} ';'
----------------------------------------------------------
... sorry...
Comment 20 Victor Bogado 2008-04-25 14:34:41 EDT
There alteration made. New specs and rpms : 

http://bogado.net/rpm/cwiid.spec
http://bogado.net/rpm/cwiid-0.6.00-6.bog8.src.rpm

I ran rpmlint against the newly made rpm as that : 

$ rpmlint /home/bogado/rpmbuild/RPMS/x86_64/cwiid-*
cwiid.x86_64: W: incoherent-version-in-changelog 0.6.00-6 0.6.00-6.bog8
cwiid-devel.x86_64: W: no-documentation
cwiid-python2.x86_64: W: no-documentation


Comment 21 Mamoru TASAKA 2008-04-26 02:02:34 EDT
Okay.
- This package is now clean
- Your another review request seems good to some extent (some
  fix seems needed although)

---------------------------------------------------------------------
     This package (cwiid) is APPROVED by me
---------------------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account".
At a point a mail should be sent to sponsor members which notifies
that you need a sponsor. At the stage, please also write on
this bug for confirmation that you requested for sponsorship and
your FAS (Fedora Account System) name. Then I will sponsor you.

If you want to import this package into Fedora 7/8, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.
Comment 22 Mamoru TASAKA 2008-04-27 12:06:36 EDT
Now I am sponsoring you. Please follow "Join" wiki again.
Comment 23 Victor Bogado 2008-04-27 13:45:02 EDT
Not sure I followed that last direction, "Join" wiki. :-/ 
Comment 24 Victor Bogado 2008-04-27 13:59:37 EDT
New Package CVS Request
=======================
Package Name: cwiid
Short Description: Wiimote interface library
Owners: bogado
Branches: F-9, F-9
InitialCC: bogado
Cvsextras Commits: yes
Comment 25 Victor Bogado 2008-04-27 14:00:27 EDT
New Package CVS Request
=======================
Package Name: cwiid
Short Description: Wiimote interface library
Owners: bogado
Branches: F-8, F-9
InitialCC: bogado
Cvsextras Commits: yes
Comment 26 Kevin Fenzi 2008-04-28 12:21:50 EDT
cvs done.
Comment 27 Mamoru TASAKA 2008-04-29 02:31:30 EDT
(For review request please leave assignee to the reviewer.
 Just changing assignee)
Comment 28 Victor Bogado 2008-04-29 06:45:12 EDT
Ops, sorry, didn't know this rule. :-) 
Comment 29 Victor Bogado 2008-04-30 08:51:10 EDT
I have a question, the package has a binary file '/usr/bin/lswm' in the main
package, since it is a library package yum install both .i386 and .x86_64. It
does happen seamless, I tested in my computer. But the files in the i386 package
and the x86_64 are different, so how does this work. 

I am thinking in renaming cwiid-wmgui to cwiid-utils and include this wmls in it
to solve this "problem". 
Comment 30 Mamoru TASAKA 2008-04-30 09:05:08 EDT
General solution is to
- create cwiid-libs subpackage
- move %_libdir/libcwiid.so.* to cwiid-libs
- Make cwiid package have "Requires: %{name}-libs = %{version}-%{release}"
Comment 31 Victor Bogado 2008-04-30 10:08:13 EDT
My reasoning was that both wmgui and lswm are optional tools to debug
connections and find out bluetooth ids for your wiimotes, so they are optional.
So in my idea we would have : 

cwiid - libraries.
cwiid-devel - development package.
cwiid-phyton2 - python binding.
cwiid-utils - lswm and wmgui, both utilities to debug connections.
cwiid-wminput - simulate mouse and inputs with the wiimote, an end user would
probably want this package. 
Comment 32 Mamoru TASAKA 2008-04-30 10:55:46 EDT
As this package (cwiid) is already in F-8 testing so changing rpm
name like cwiid-wmgui -> cwiid-utils needs proper handing (adding 
Provides/Obsoletes) and unless it is avoidable it should not be done.

My way (comment 30) is more safer way.
Comment 33 Victor Bogado 2008-04-30 11:31:10 EDT
Other option would be create another package cwiid-lswm with only this file. But
I thought that this would bloat the cwiid namespace. 

I think that this util 'lswm' is mostly a test thingy that is useless 90%-99% of
the time. Having a package for it alone is an overkill. I get it that renaming
is a hard option right now, and I regret have not seeing this before. So if we
let out the renaming option I have basically two options : 

your idea on #30 :
cwiid - only packages /usr/bin/lswm or maybe even nothing on it. this packagfe
requires cwiid-libs.
cwiid-libs - runtime library stuff
cwiid-devel - development stuff
cwiid-lswm - /usr/bin/lswm in the case it is not packaged in the cwiid package.

or : 
cwiid - runtime library stuff (as it is now, but without the lswm util)
cwiid-* - not changed
cwiid-lswm - new package with /usr/bin/lswm.

I tend to think that #2 is more to the point, even though it has more packages.
Comment 34 Mamoru TASAKA 2008-04-30 11:43:19 EDT
Would you tell me what rpms you think the plugins under %_libdir/%name
belong to?
Comment 35 Victor Bogado 2008-04-30 11:56:03 EDT
wminput, this is a mistake also. :P Damn I am stupid. 
Comment 36 Mamoru TASAKA 2008-04-30 13:33:30 EDT
Created attachment 304260 [details]
proposal spec file

Well, then it leaves cwiid-lswm only one file, too bad...
Let's adopt your original idea (comment 29). Proposal spec file
attached, change the description if you like.
Comment 37 Victor Bogado 2008-04-30 14:03:07 EDT
Great. :-) I'll import this into my git and test it later. 

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