This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours

Bug 483025

Summary: Review Request: imms - Adaptive playlist framework tracking and adapting to your listening patterns
Product: [Fedora] Fedora Reporter: Milos Jakubicek <xjakub>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, notting, tcallawa
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-11-08 22:06:02 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On: 483020, 555325    
Bug Blocks: 201449    
Attachments:
Description Flags
patch to fix plugin preferences dialog none

Description Milos Jakubicek 2009-01-29 07:18:36 EST
Spec URL: http://mjakubicek.fedorapeople.org/imms/imms.spec
SRPM URL: http://mjakubicek.fedorapeople.org/imms/imms-3.1.0-0.1.rc6.fc10.src.rpm
Description: IMMS is an adaptive playlist framework that tracks your listening patterns and dynamically adapts to your taste. Currently we ship only the XMMS plugin.

Its major features include:

* IMMS is easy to install. It is purely a plugin - no XMMS patch required.
  A very lightweight embedded SQL database is used, so there's no need to setup
  a RDBMS.
* IMMS is easy to use. Song rating is done completely transparently
  to the user. It does not get in your way.
* IMMS does a much better job of shuffling than most players. It keeps track
  of when a song was last played, and makes sure same songs are not repeated
  too often. It is even able to recognise different versions of the same song
  (eg. remixes) and treat them as one song!
* IMMS uses a variety of techniques to figure out which songs should be played
  together, and which should not. It studies your listening habits, as well as
  using acoustic properties of the songs themselves, such as BPM and frequency
  spectrum.
* IMMS is fair. Even unfavoured songs have a (small) chance of being played.

There are some outstanding issues I have to deal with and would appreciate any help:

1) rpmlint complains about executable-stack:

>rpmlint imms-3.1.0-0.1.rc6.fc10.x86_64.rpm
imms.x86_64: W: executable-stack /usr/bin/immsd
imms.x86_64: W: executable-stack /usr/bin/immstool
imms.x86_64: W: xmms-naming-policy-not-applied /usr/lib64/xmms/General/libxmmsimms.so

The naming policy is not a problem imho, but I don't know how to get rid of the executable-stack warning.

2) Licensing issues

The project claims to be GPL, but this license is not specified in source file. Moreover it includes some (modified) third-party code (see AUTHORS) -- this is mostly ok (better than own source files, it includes the license) except of:

immscore/xidle.c
model/emd.c
immscore/normal.h

These files and those written by the imms upstream need definitely a license specified -- I'm going to contact the author and setting FE-LEGAL for now.
Comment 1 Milos Jakubicek 2009-01-30 08:26:23 EST
Torch is already built in rawhide, hence I made a scratch build here:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1093697

I also added all the missing BR, had to add also automake + autoconf because of the funny configure script provided in the source tarball:

>cat configure
#!/bin/sh
rm $0
make
$0 $*

New SPEC: http://mjakubicek.fedorapeople.org/imms/imms.spec
New SRPM: http://mjakubicek.fedorapeople.org/imms/imms-3.1.0-0.2.rc6.fc10.src.rpm
Comment 2 Tom "spot" Callaway 2009-02-12 13:26:30 EST
Any update on the licensing issue?
Comment 3 Milos Jakubicek 2009-02-13 18:10:11 EST
Yep, I'm quoting the answer to my email:

"
Hey, Milos,

I fixed compilation with recent Audacious versions in 3.1.0-rc7.
[http://www.luminal.org/files/imms/imms-3.1.0-rc7.tar.bz2]

I am fairly sure all the code in question is GPL. I made sure of that
before including it.
I do not have the time to follow up with the authors of the code right
now, but I'm certainly willing to apply any patches to clarify
licenses.
Perhaps, Artur, the Debian IMMS maintainer (cc'ed) can help you there.
He already did some work on getting explicit releases from the
authors.

Have fun,
Michael. 
"

It seems that all the thins are GPL indeed...looking into debian(lenny) imms packages, I found following:

"
+Author: Michael Grigoriev <mag@luminal.org>
+
+Copyright (C) 2003-2005 Michael Grigoriev
+
+With the following exceptions:
+
+    md5.{h,c} (added ability to restrict maximum size to process)
+        from GNU textutils
+    levenshtein.{h,c} (stripped down)
+        python-Levenshtein library
+        by David Necas (Yeti) <yeti@physics.muni.cz>
+    regexx.{h,cc} (stripped down)
+        Regexx - Regular Expressions C++ solution
+        by Gustavo Niemeyer <gustavo@nn.com.br>
+    xidle.{h,cc}
+        Borrows from xautolock 2.1
+        by Michel Eyckmans (MCE) <eyckmans@imec.be>
+    normal.h
+        by by Agner Fog
"

I'll prepare patches and request adding a LICENSE file into the source tarball and license information to all the source files (and merging the gcc43 patch of course too). I'd also include the above information as %doc. Would it be ok then?

Regarding the review: the audacious plugin is working now, updated link are here:

New SPEC: http://mjakubicek.fedorapeople.org/imms/imms.spec
New SRPM: http://mjakubicek.fedorapeople.org/imms/imms-3.1.0-0.3.rc7.fc10.src.rpm
Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1125850

As there are now both xmms and audacious plugins available, I'm considering splitting the package into a main one and two subpackages (-xmms, -audacious), I'm just not sure whether this wouldn't hit the usability from the user point of view too much.
Comment 4 Tom "spot" Callaway 2009-02-14 08:56:44 EST
Yeah, this is sufficient for Fedora Legal. Lifting FE-Legal.
Comment 5 Milos Jakubicek 2009-02-15 19:45:48 EST
OK, upstream is very responsive, hence there is a new (pre)release with all the necessary licensing info:

New SPEC: http://mjakubicek.fedorapeople.org/imms/imms.spec
New SRPM:
http://mjakubicek.fedorapeople.org/imms/imms-3.1.0-0.4.rc8.fc10.src.rpm

Moreover, the GCC 4.3 patch has been merged upstream and the rpmlint issue with executable stack has been also clarified, it is caused by a objdump call (details in specfile) and has been simply fixed by removing the flag using execstack.

I've also split the plugins into a xmms-imms and audacious-plugins-imms subpackage.

So this is now definitely ready for a review.
Comment 6 Jason Tibbitts 2009-03-13 16:20:13 EDT
Unfortunately it fails to build for me:

+ execstack -c build/immsd build/immstool
RPM build errors:
/var/tmp/rpm-tmp.TIGCLm: line 41: execstack: command not found
error: Bad exit status from /var/tmp/rpm-tmp.TIGCLm (%install)
Comment 7 Milos Jakubicek 2009-03-13 20:38:48 EDT
Ops, forgot to BR prelink after adding the execstack call, sorry.

New SPEC: http://mjakubicek.fedorapeople.org/imms/imms.spec
New SRPM: http://mjakubicek.fedorapeople.org/imms/imms-3.1.0-0.5.rc8.fc10.src.rpm
Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1240946
Comment 9 Milos Jakubicek 2009-06-06 11:42:03 EDT
wrong link by copy&paste in the previous comment:
http://mjakubicek.fedorapeople.org/imms/imms-3.1.0-0.6.rc8.fc10.src.rpm
Comment 10 Milos Jakubicek 2009-08-25 21:17:17 EDT
New Koji scratch build in case someone would review this:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1631677
Comment 11 Michael Schwendt 2010-01-14 06:57:33 EST
* spectool -g imms-3.1.0-0.6.rc8.fc11.src/imms.spec
retrieves an index.html file instead of the source tarball.
Project hosting has moved to Google Code.


* Currently fails to build (F-12 i686):

| audplugin.o: In function `imms_get_playlist_item(int)':
| /home/qa/mnt/work/tmp/rpm/BUILD/imms-3.1.0-rc8/build/../clients/audacious/audplugin.cc:81: undefined reference to `filename_to_utf8'
| collect2: ld returned 1 exit status
| make[1]: *** [libaudaciousimms.so] Error 1

Seems it doesn't link libaudcore and would need an update for Audacious >= 2.1 on F-12 (for F-12 there is a koji buildroot override tag for Audacious 2.2 currently, btw).


* The spec file isn't pretty due to questionable sed substitutions. What do you do if some of the matches fail silently? Only if that results in a failing build, the sed usage is acceptable. Else you ought to prefer patch files. (or add guards in the spec file)

Examples:

1) Using sed to modify install paths => upstream release modifies the build framework => your sed subst fails => rpmbuild fails because %install fails or the buildroot contents don't match the %files section => ACCEPTABLE, because you cannot proceed without fixing your package so it will build again.

2) Using sed to modify the contents of files => upstream release modifies the files => your sed subst fails => rpmbuild succeeds because no build error is found => NOT ACCEPTABLE, because either your sed substs are obsolete OR they fail silently => in case of the latter they don't apply your fixes anymore.


In %build you use sed to adjust various CPPFLAGS/CXXFLAGS prior to running %configure. Nothing verifies that your sed substitutions have been applied actually. Bad.

In %install you use sed to adjust install paths to point them into the buildroot. Consider yourself lucky that if the sed substitutions fail, most likely the rpmbuild fails too, because %files doesn't match buildroot anymore. Generally, however, there is no guarantee. And you use wildcards in the %files section, which means you don't even require specific files to be present in the buildroot ('*' = any file that's found will match). In the combination with sed substs, this is unsafe. Safer would be to hardcode the names of important files (e.g. immsd and immstool) and effectively require them to be found.


* BuildRequires:  sox

What about run-time? I see in the source that it tries to popen() sox at run-time. Does that result in a run-time requirement of sox? Or is it fully optional and with a clear error/warning message if sox cannot be found?


* As a hint for the %files section: where you include entire directories
recursively, such as 

  %{_datadir}/imms

it's more readable to append a slash like

  %{_datadir}/imms/

to make explicit that it's a directory and not a single file.
Comment 12 Milos Jakubicek 2010-01-14 07:42:11 EST
Michael, thank you for looking at this, this is already quite ancient, many things changed meanwhile as you can see, will look at it.
Comment 13 Milos Jakubicek 2010-01-14 08:33:11 EST
(In reply to comment #11)

> * Currently fails to build (F-12 i686):
> 
> | audplugin.o: In function `imms_get_playlist_item(int)':
> |
> /home/qa/mnt/work/tmp/rpm/BUILD/imms-3.1.0-rc8/build/../clients/audacious/audplugin.cc:81:
> undefined reference to `filename_to_utf8'
> | collect2: ld returned 1 exit status
> | make[1]: *** [libaudaciousimms.so] Error 1


Ah...I filed a bug for audacious and find out you're the maintainer...so this just bounces back, sorry:)
Comment 14 Milos Jakubicek 2010-01-16 20:59:46 EST
New build:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1927651

* nasty seds: those for compilation and linking flags have been turned into a patch, those for build root harden by removing any wildcards from the %files section.

* BuildRequires:  sox

GOOD CATCH! Thanks for that! (fixed)

* As a hint for the %files section...

Done as well...anything what improves readability is a good point, always, thanks.

* Also added xmms as Requires: for the -xmms subpackage. I remember that previously it was pulled in automagically through imms-libs, but is not anymore.
Comment 15 Michael Schwendt 2010-01-17 17:04:17 EST
Created attachment 384970 [details]
patch to fix plugin preferences dialog

Without the attached patch, the Audacious IMMS plugin's Preferences dialog cannot be placed in front of other windows (even if it has focus).

[...]

As I probably won't have time to continue with reviewing until end of next week, just some comments on a look at IMMS run-time:

$ audacious
imms client: Connection failed: Connection refused
immsd: version 3.1.0-rc8 ready...
immsd: another instance already active - exiting.

* As it successfully starts an "immsd" instance, so no idea why it warns about another instance.


* /usr/bin/analyzer currently doesn't conflict with anything in Fedora, but it is reason to be concerned about a future conflict. Why the heck isn't it called "immsanalyzer" with the same "imms" namespace as the other executables?


* Fedora's "sox" as used by /usr/bin/analyzer fails to handle .mp3, .mpc, or .wma, for example. It seems there are no ready-to-use add-on packages for sox in 3rd party repos.   It's happy about .ogg, .flac, .wav however.
Comment 16 Jason Tibbitts 2010-11-02 20:35:16 EDT
It's been the better part of a year since the last comment with no response.  I'm thinking that at this point this should just be closed, and I'll do that soon if nothing happens.
Comment 17 Jason Tibbitts 2010-11-08 22:06:02 EST
No response; closing.