-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fetch SUSE and Spotify versions on the fly. #37
base: master
Are you sure you want to change the base?
Conversation
Thanks! This approach sounds much more future-proof. I have some nitpicks though ... |
I think it would be better to squash all 3 commits into a single one (see this guide if you need help with that). |
I made some updates based on your suggestions and squashed them all into a single commit. Let me know what you think. =) |
@@ -31,7 +31,6 @@ Requires: mozilla-nspr | |||
Requires: libopenssl1_0_0 | |||
Requires: libpng12-0 | |||
Recommends: libmp3lame0 | |||
Recommends: ffmpeg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't follow why you removed this, as it goes directly against #35.
Awesome, thanks! We're very close now - just indentation and the |
Whoops, not sure what happened there. |
I didn't delete the ffmpeg recommendation, I think I just created my fork before that change was added, and I forgot to merge new changes from your branch in. In any case, I added ffmpeg back in, cleaned up the tabs, and squashed everything into one commit, so hopefully it looks good to go. |
Thanks! Taking a look ... |
Oh crap, I just realised that whilst your dynamic approach to determining the version is great for the I can think of the following possible solutions:
Option 5 is obviously the right thing to do. However, since we are weak and foolish humans, maybe option 2 is the most realistic solution for the short-term, and option 4 for the long term. Thoughts? |
Hmm, just occurred to me that merging this PR would mean that one version of the |
This pull request was spot on as I had cloned the Repo and had issues until I saw this. So thanks! As far as resolving #33. I just used the opensuse package for libgcrypt.so.11 provided here: https://software.opensuse.org/package/libgcrypt11 ( which it seems you are already aware of) Are there plans afoot to incorporate the zypper check of libgcrypt.so.11 and get it if it doesn't exist? mentioned on #38 |
@dradtke Do you have any thoughts on my comments? |
@bclouser Thanks for the info but please let's not pollute this issue with discussion of unrelated topics - I'm already confused enough as it is ;-) |
Any idea when this will be integrated? The current version is too old. |
Yes please. It would be great to see this PR merged! |
This PR removes the hard-coded SUSE and Spotify versions, and instead parses
/etc/os-release
andhttp://repository.spotify.com/pool/non-free/s/spotify
respectively at the time the installation is run. With these changes I was able to get Spotify installed on openSUSE 13.2, and I suspect that it will work for future versions as well.