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

[RPM Fusion Bugzilla]

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

--- Comment #10 from Jonathan Dieter <jdieter at gmail.com> 2012-11-12 17:18:40 CET ---
(In reply to comment #9)
> (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.

The thing is, the target files of the symlinks are required in the rpm, so I
think both ways the risk is install-time.  If that's the case, I'd definitely
prefer the dangling symlinks.

> > 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? 

Rpm's built-in filtering tries to require the libraries ending in .0d and .1d
because that's what they are in spotify and libcef.so.  My scripts call rpm's
built in scripts and then filter out the .0d and .1d, making the file requires
match what's provided by the nss and nspr packages.

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

Fair enough.  Honestly, I was just going to ignore it until I realized that I
couldn't build the i686 rpm on my x86_64 laptop with x86_64 chrpath installed.

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

Great!

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