[Bug 2565] Review-Request: spotify-client - streaming music player

[RPM Fusion Bugzilla]

https://bugzilla.rpmfusion.org/show_bug.cgi?id=2565

--- Comment #9 from Alec Leamas <leamas.alec at gmail.com> 2012-11-12 16:45:26 CET ---
(In reply to comment #8)
Thanks for taking time for this! Here we go:

> Ok, I've looked at this and found a number of things that I think need to be
> changed and have tested to make sure the changes make sense:
> 
> 1. I'm not a huge fan of %ghosted files and installing symlinks in %post.  I
> think it makes more sense to install the symlinks in %install, have them
> dangle, and just make sure we require the right files (which we have to do
> anyway).
I have considered a (and actually implemented() both ways. To me it seems that
the risk with using dangling symlinks is a run-time failure whereas the risk
with creating links in %post is an install-time one; of these I preferred the
last. The obvious downside is added complexity in the spec. Given this, if you
still insist to use the "dangling symlinks" approach I'm certainly open to
that.

> 2. Rather than owning each individual file in %{_libdir}/spotify-client, I
> think it makes the more sense to just own the directory.
Agreed if we goes for the dangling symlinks approach.

> 3. The problems listed in comments #6 and #7 were mainly because libcef.so
> wasn't marked as executable, so rpmbuild wasn't properly generating the
> automatic build requires.  
Ah... nice finding!

> By making libcef.so executable and creating custom requires 
> and provides scripts, we get all the requires listed properly, and, as
> a bonus, no longer have to manually require the symlinked libraries. 
What's the advantage to use separate find_provides/find_requires scripts
instead of using rpm's built-in filtering? 

> 4. Rpaths are bad.  See
> https://fedoraproject.org/wiki/Packaging:Guidelines#Beware_of_Rpath.  
Actually, I disagree. Rpaths to *private* as this libs are not bad, see next
chapter
https://fedoraproject.org/wiki/Packaging:Guidelines#Rpath_for_Internal_Libraries

> To top it
> off, chrpath only works on the arch that the binary was built for, so you can
> use x86_64 chrpath to change a i686 binary's rpath.  This may or may not cause
> problems with RPM Fusion's builders.  At the very least, it prevents a user
> from building a i686 package on an x86_64 system.  Using LD_LIBRARY_PATH in
> spotify.sh achieves the same result without needing chrpath.
OTOH, using LD_LIBRARY_PATH is generally frowned upon. Still, I have considered
using it anyway for the simple reason that using chrpath means that we actually
change the binary which might become a licensing issue if the licensing
conditions are clarified upstream. 

Bottom line: I buy your conclusion, but not the argument that a rpath is bad in
this case.

> 5. I think it makes sense to have spotify.sh as a separate %SOURCE rather than
> embedded in the spec, mainly for neatness' sake.
Agreed.

> An updated SPEC which incorporates all of the above is here:
> Spec: http://lesloueizeh.com/jdieter/spotify-client.spec
> SRPM:
> http://lesloueizeh.com/jdieter/spotify-client-0.8.4.103.g9cb177b.260-2.fc17.src.rpm

Thanks. Will incorporate those changes in a new version after sorting out open
issues above. Also have some other changes pending (updated wrapper script,
manpage).

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