Bug 483025

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

Any update on the licensing issue?

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>
+
+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.cz>
+    regexx.{h,cc} (stripped down)
+        Regexx - Regular Expressions C++ solution
+        by Gustavo Niemeyer <gustavo.br>
+    xidle.{h,cc}
+        Borrows from xautolock 2.1
+        by Michel Eyckmans (MCE) <eyckmans>
+    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.

Yeah, this is sufficient for Fedora Legal. Lifting FE-Legal.

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.

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)

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

Rebuilt, solved the ownership of audacious/General directory:

http://mjakubicek.fedorapeople.org/imms/imms.spec
http://mjakubicek.fedorapeople.org/imms/imms-3.1.0-0.5.rc8.fc10.src.rpm
http://koji.fedoraproject.org/koji/taskinfo?taskID=1396981

wrong link by copy&paste in the previous comment:
http://mjakubicek.fedorapeople.org/imms/imms-3.1.0-0.6.rc8.fc10.src.rpm

New Koji scratch build in case someone would review this:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1631677

* 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.

Michael, thank you for looking at this, this is already quite ancient, many things changed meanwhile as you can see, will look at it.

(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:)

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.

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.

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.

No response; closing.