Bug 552113 - Review Request: wiiuse - library to use wiiremotes via bluetooth
Summary: Review Request: wiiuse - library to use wiiremotes via bluetooth
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jason Tibbitts
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-01-04 04:37 UTC by Simo Sorce
Modified: 2011-04-08 15:46 UTC (History)
4 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2011-04-08 15:46:41 UTC
Type: ---
Embargoed:
j: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Simo Sorce 2010-01-04 04:37:15 UTC
Spec URL: http://simo.fedorapeople.org/libwiiuse/wiiuse.spec
SRPM URL: http://simo.fedorapeople.org/libwiiuse/libwiiuse-0.12-0.fc11.src.rpm
Description: A library that implements access to wiiremote controllers via bluetooth.

Comment 1 Kris Buytaert 2010-01-06 22:16:09 UTC
First quick look :

$ rpmlint  libwiiuse-0.12-0.fc11.src.rpm  
libwiiuse.src: E: invalid-spec-name
1 packages and 0 specfiles checked; 1 errors, 0 warnings.
$ rpmlint  wiiuse.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.



Package fails against : 

MUST: The spec file name must match the base package %{name},

Comment 2 Simo Sorce 2010-01-06 22:42:48 UTC
oops sorry, I forgot to rename it after I decided to rename the package from wiiuse to libwiiuse.

I have uploaded a new src.rpm and spec file with the right name.

Comment 3 Rich Mattes 2010-01-12 03:57:24 UTC
The specfile url should point to libwiiuse.spec now, no?

The wiiuse website says that it's dual-licensed under GPLv3 and LGPLv3, but the spec only mentions GPLv3.

Mock build works for me (fedora-12-i386)

rpmlint output:
$ rpmlint libwiiuse.spec ../RPMS/i686/libwiiuse-*
libwiiuse.i686: W: unstripped-binary-or-object /usr/lib/libwiiuse.so
libwiiuse.i686: W: no-soname /usr/lib/libwiiuse.so
libwiiuse.i686: W: wrong-file-end-of-line-encoding /usr/share/doc/libwiiuse-0.12/CHANGELOG
libwiiuse.i686: W: wrong-file-end-of-line-encoding /usr/share/doc/libwiiuse-0.12/README
libwiiuse.i686: W: one-line-command-in-%post /sbin/ldconfig
libwiiuse.i686: W: one-line-command-in-%postun /sbin/ldconfig
libwiiuse-debuginfo.i686: E: empty-debuginfo-package
libwiiuse-devel.i686: W: no-documentation
libwiiuse-examples.i686: W: unstripped-binary-or-object /usr/bin/wiiuse-example
libwiiuse-examples.i686: W: unstripped-binary-or-object /usr/bin/wiiuse-sdl
libwiiuse-examples.i686: W: no-documentation
4 packages and 1 specfiles checked; 1 errors, 10 warnings.


The %post problems are easy to fix with
%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig

The wrong file-end-of-line-encoding can be fixed with dos2unix.  The rest could be because of the build system

Comment 4 Simo Sorce 2010-01-12 13:57:28 UTC
(In reply to comment #3)
> The specfile url should point to libwiiuse.spec now, no?

Yes, should I change it? If so how ?

> The wiiuse website says that it's dual-licensed under GPLv3 and LGPLv3, but the spec only mentions GPLv3.

Yes the reason is that the LGPLv3 version is a modified *non-free* version (LGPLv3 is granted only for non-commercial purposes).
So I choose to use the only free version we are allowed to use in Fedora.

> Mock build works for me (fedora-12-i386)
> 
> rpmlint output:
> $ rpmlint libwiiuse.spec ../RPMS/i686/libwiiuse-*
> libwiiuse.i686: W: unstripped-binary-or-object /usr/lib/libwiiuse.so

Doesn't rpm strip binaries on its own after building debuginfo packages?
Or do I need to actively strip them ?

> libwiiuse.i686: W: no-soname /usr/lib/libwiiuse.so
Not sure how to fix this one.

> libwiiuse.i686: W: wrong-file-end-of-line-encoding
> /usr/share/doc/libwiiuse-0.12/CHANGELOG
> libwiiuse.i686: W: wrong-file-end-of-line-encoding
> /usr/share/doc/libwiiuse-0.12/README

All the files in this library have \r\m line endings and are DOS encoded.
Should I change that ? How ?

> libwiiuse.i686: W: one-line-command-in-%post /sbin/ldconfig
> libwiiuse.i686: W: one-line-command-in-%postun /sbin/ldconfig
> libwiiuse-debuginfo.i686: E: empty-debuginfo-package

How do I fix this ?

> libwiiuse-devel.i686: W: no-documentation
> libwiiuse-examples.i686: W: unstripped-binary-or-object /usr/bin/wiiuse-example
> libwiiuse-examples.i686: W: unstripped-binary-or-object /usr/bin/wiiuse-sdl
> libwiiuse-examples.i686: W: no-documentation
> 4 packages and 1 specfiles checked; 1 errors, 10 warnings.

> The %post problems are easy to fix with
> %post -p /sbin/ldconfig
> %postun -p /sbin/ldconfig

I will fix this one.

> The wrong file-end-of-line-encoding can be fixed with dos2unix.

Do I really need to invoke dos2unix at build time to fix them ?

> The rest could be because of the build system    

Probably, I didn't use mock to build the srpm.

Let me know if there docs or howtos to read that explain the above issues and how to fix them (rpmlint on F11 didn't complain at all)

Comment 5 Rich Mattes 2010-01-12 16:15:00 UTC
I was just making sure that I was looking at the right files.  The licensing thing also makes sense.

Some comments on the spec: 

- According to http://fedoraproject.org/wiki/Packaging/Debuginfo, shared libs and binaries should be exectuable.  the .so and two programs should be installed with permission 755, and the header file can stay 644.  This clears up the debuginfo warning and the unstripped binary warnings.

- %post -p and %postun -p do clear up the rpmlint warning 

- dos2unix fixes the line encoding.  The solution at https://fedoraproject.org/wiki/ParagNemade/CommonRpmlintErrors#wrong-script-end-of-line-encoding is to not include files like that at all in the source tarballs, i don't know how feasible that is.

- The no documentation messages should be OK, you have some documentation in the base package.

- The no-soname error means that the library is being built without a soname.  One way to get around this is to add an option to the LDFLAGS in src/Makefile.  You can add -Wl,-soname,libwiiuse.so to give the library the soname of "libwiiuse.so".  Verify by using "objdump -p libwiiuse.so |grep SONAME"  

Other comments: 

- Standard build systems like autotools and cmake play nicer with rpms, but with some patching this one could be made to work w.r.t. install paths, soname generation, etc.

- It would be nice if there was a .pc file included as it's a development library.

Comment 6 Simo Sorce 2010-01-12 16:42:15 UTC
(In reply to comment #5)
> I was just making sure that I was looking at the right files.  The licensing
> thing also makes sense.

OK

> Some comments on the spec: 
> 
> - According to http://fedoraproject.org/wiki/Packaging/Debuginfo, shared libs
> and binaries should be exectuable.  the .so and two programs should be
> installed with permission 755, and the header file can stay 644.  This clears
> up the debuginfo warning and the unstripped binary warnings.

will fix post install

> - %post -p and %postun -p do clear up the rpmlint warning 

will do

> - dos2unix fixes the line encoding.  The solution at
> https://fedoraproject.org/wiki/ParagNemade/CommonRpmlintErrors#wrong-script-end-of-line-encoding
> is to not include files like that at all in the source tarballs, i don't know
> how feasible that is.

I don't think it is very feasible, every single text file including source code is in DOS format :(

> - The no documentation messages should be OK, you have some documentation in
> the base package.
> 
> - The no-soname error means that the library is being built without a soname. 
> One way to get around this is to add an option to the LDFLAGS in src/Makefile. 
> You can add -Wl,-soname,libwiiuse.so to give the library the soname of
> "libwiiuse.so".  Verify by using "objdump -p libwiiuse.so |grep SONAME"  

will do

> Other comments: 
> 
> - Standard build systems like autotools and cmake play nicer with rpms, but
> with some patching this one could be made to work w.r.t. install paths, soname
> generation, etc.

Yeah the quality of the build system is craptastic, but it is what it is for now.

> - It would be nice if there was a .pc file included as it's a development
> library.    

At some point I will try to contribute something like this to upstream, for now I guess we need to settle with what is available, as long as it is acceptable.

Comment 7 Jason Tibbitts 2010-11-16 20:05:34 UTC
Did anything ever happen with this package?  I see plenty of commentary but no updated srpm.

Comment 8 Simo Sorce 2010-11-16 20:34:59 UTC
I've got distracted and haven't updated it.
Will see if I can find out some time in the week end.

Comment 9 Jason Tibbitts 2010-11-16 21:09:29 UTC
Cool, thanks.

Comment 10 Simo Sorce 2010-11-20 23:32:11 UTC
Ok I have uploaded new spec file and src.rpm to http://simo.fedorapeople.org/libwiiuse/

This should solve the problems reported earlier.
I still have this though:
> libwiiuse-debuginfo.i686: E: empty-debuginfo-package

Not sure how to solve that.

Comment 11 Jason Tibbitts 2010-11-22 18:18:21 UTC
The "empty-debuginfo-package" thing is probably a symptom of the package not building with the proper compiler flags.  Either the flags need to be passed to make or the Makefile needs patching.

Comment 12 Jason Tibbitts 2010-11-25 18:27:59 UTC
I did take a look at the Makefile and it looks that even though you can pass CFLAGS, it will only use it for the final link and not building any of the source files.  You'll have to sed or patch out the CFLAGS manipulation in each of the Makefiles.

Comment 13 Simo Sorce 2010-11-25 20:19:09 UTC
Ok I added a patch (wiiuse_cflags.patch) so that the makefiles do not reset cflags but add to environment defined ones.

Setting CFALGS=-g in the shell before running rpmbuild now generated non-empty debuginfo files. I guess this means it will properly pick up whatever CFLAGS are set in the koji environment.

Comment 14 Jason Tibbitts 2010-11-26 14:38:40 UTC
Well, there is no default CFLAGS set in the koji environment.  The only environment is what you specify in your spec file.  Note that usually the %configure macro sets CFLAGS, which may be what you're used to, but of course this software doesn't use autoconf.

So, you probably need to stick CFLAGS='%optflags' on your make line.

If you post an updated package I'll take a look.  All I see is the same release 0 package I already looked at.  (Of course, your release should start at 1, and you should increment it whenever you update the package, even during review.)

Comment 15 Simo Sorce 2010-11-26 15:36:19 UTC
Ok now passing CFLAGS to make in %build and bumped up release number.
Let me know if there is anything else to change.

Comment 16 Jason Tibbitts 2010-12-01 02:24:58 UTC
Could you elaborate on why you chose to rename from wiiuse to libwiiuse?  The upstream project name seems to be wiiuse.

The summary of the main package could use some work.  It doesn't say anything that the name of the package doesn't.  I'd use something like "A library that connects with several Nintendo Wii remotes".

The license seems to be GPLv3+, not GPLv3.  At least the source says so rather explicitly.  Do you have some other source of information that indicates that the license is GPLv3 only?  (Also, I asked the legal list about the LGPLv3/noncommercial thing and it appears that the best thing to do is ignore it completely.) 

* source files match upstream.  sha256sum:
  afc86b05ab201842c7f258e3d854171ede80167bd257272ae59d0cfbd342fb0d
   wiiuse_v0.12_src.tar.gz
? package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
X summary could use some work.
* description is OK.
* dist tag is present.
X license field does not match the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* debuginfo package looks complete.
* rpmlint has acceptable complaints.
* final provides and requires are sane:
  libwiiuse-0.12-1.fc15.x86_64.rpm
   libwiiuse.so.0()(64bit)  
   libwiiuse = 0.12-1.fc15
   libwiiuse(x86-64) = 0.12-1.fc15
  =
   /sbin/ldconfig  
   libbluetooth.so.3()(64bit)  

  libwiiuse-devel-0.12-1.fc15.x86_64.rpm
   libwiiuse-devel = 0.12-1.fc15
   libwiiuse-devel(x86-64) = 0.12-1.fc15
  =
   libwiiuse = 0.12-1.fc15
   libwiiuse.so.0()(64bit)  

  libwiiuse-examples-0.12-1.fc15.x86_64.rpm
   libwiiuse-examples = 0.12-1.fc15
   libwiiuse-examples(x86-64) = 0.12-1.fc15
  =
   libGL.so.1()(64bit)  
   libGLU.so.1()(64bit)  
   libSDL-1.2.so.0()(64bit)  
   libbluetooth.so.3()(64bit)  
   libglut.so.3()(64bit)  
   libwiiuse = 0.12-1.fc15
   libwiiuse.so.0()(64bit)  

* no bundled libraries.
* shared libraries installed; ldconfig called properly.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files
* scriptlets are OK (ldconfig).
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* headers are in the -devel package.
* no libtool .la files.

Comment 17 Simo Sorce 2010-12-12 20:38:48 UTC
(In reply to comment #16)
> Could you elaborate on why you chose to rename from wiiuse to libwiiuse?  The
> upstream project name seems to be wiiuse.

No special reason, I think debian packages maybe calling it so, and because it was just a library I prepended lib to the name.

I renamed the package, the spec, srpm, source and patches are now available here:
http://simo.fedorapeople.org/wiiuse

> The summary of the main package could use some work.  It doesn't say anything
> that the name of the package doesn't.  I'd use something like "A library that
> connects with several Nintendo Wii remotes".

Ok, I expanded the summary.

> The license seems to be GPLv3+, not GPLv3.  At least the source says so rather
> explicitly.  Do you have some other source of information that indicates that
> the license is GPLv3 only?  (Also, I asked the legal list about the
> LGPLv3/noncommercial thing and it appears that the best thing to do is ignore
> it completely.) 

I probably just missed the (or later) clause the first time I read the license, then got busy evaluating whether the LGPLv3 part could be used or not in Fedora and came to the conclusion it was not free and therefore ignore it. (I am glad Legal came to the same conclusion :)

Fixed to GPLv3+

Hopefully I addressed all remaining concerns.

Comment 18 Jason Tibbitts 2010-12-15 20:44:21 UTC
I don't think it's a significant problem that you renamed it, although we do try to stick with the upstream names when it makes sense.  Either way is fine with me; I was just asking for an explanation.

Unfortunately two problems were introduced.  The big one is that the packages don't install; the -devel and -examples still depend on libwiiuse.

Also, the new summary grew a trailing dot and is also somewhat overlong:

wiiuse.x86_64: W: summary-ended-with-dot C The wiiuse library is used to access and control multiple Nintendo Wiimotes, the Wii remote controllers.
wiiuse.x86_64: E: summary-too-long C The wiiuse library is used to access and control multiple Nintendo Wiimotes, the Wii remote controllers.

(not sure where rpmlint gets the 'C' from).  Honestly you can probably drop the comma and everything after if you need to free space.

Sorry for all the back and forth.

Comment 19 Simo Sorce 2011-01-16 15:49:22 UTC
Ok updated spec and srpm files in the usual place.
I fixed the broken dependencies, and the summary.

Comment 20 Jason Tibbitts 2011-01-20 03:21:12 UTC
OK, these install and the rpmlint complaints are no more.  Looks good to me.

APPROVED

Comment 21 Simo Sorce 2011-01-21 22:12:12 UTC
New Package SCM Request
=======================
Package Name: wiiuse
Short Description: The wiiuse library is used to access and control multiple Nintendo Wiimotes
Owners: simo
Branches: f15 master
InitialCC:

Comment 22 Jason Tibbitts 2011-01-21 22:14:08 UTC
It's too early to request f15 branches.  Otherwise....
Git done (by process-git-requests).

Comment 23 Jason Tibbitts 2011-04-08 15:46:41 UTC
I see no reason to keep this ticket open as the package is in rawhide and F15.


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