[Bug 483025] Review Request: imms - Adaptive playlist framework tracking and adapting to your listening patterns

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=483025

Michael Schwendt <mschwendt at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mschwendt at gmail.com

--- Comment #11 from Michael Schwendt <mschwendt at gmail.com> 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.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.