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

(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_Li... > 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.fc... 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).