Skip to content
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

Homebrew support #189

Closed
flexwie opened this issue May 5, 2023 · 11 comments
Closed

Homebrew support #189

flexwie opened this issue May 5, 2023 · 11 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@flexwie
Copy link

flexwie commented May 5, 2023

Is your feature request related to a problem? Please describe.
Using cargo to install the player is the easiest way for Mac users, but still requires to have the rust toolchain installed on the machine.

Describe the solution you'd like
Many if not most applications for Mac can be installed with Homebrew. It would be nice to have a formula to do so.

Describe alternatives you've considered
I guess a simple bash script that grabs the binaries from the releases would work as well, but its less elegant and won't support updating and uninstalling.

@flexwie flexwie added the enhancement New feature or request label May 5, 2023
@aome510 aome510 added the help wanted Extra attention is needed label May 5, 2023
@aome510
Copy link
Owner

aome510 commented May 5, 2023

I don't have any experience dealing with homebrew before. Any help on this will be greatly appreciated.

@imNel
Copy link

imNel commented May 7, 2023

Basically you'll have to create a ruby file for spotify-player and then PR into homebrew-core. You can use the docs for adding a new formula or view the formula cookbook for more info :)

@flexwie
Copy link
Author

flexwie commented May 8, 2023

I don't have any experience either so far, but I would be open to trying it out!

@tresni
Copy link

tresni commented May 16, 2023

I did this 3 weeks ago 😁 Homebrew/homebrew-core#128997

Please test and 👍 that formula to try to get it bumped up for inclusion.

@flexwie
Copy link
Author

flexwie commented May 21, 2023

Looks great! One thing I'd love to have is the ability to choose between the different features instead of installing them all. Maybe that could be done with options?

@tresni
Copy link

tresni commented May 21, 2023

That is possible. At one point I know it was being discouraged to have options in homebrew-core and instead to do that in a separate tap (similar to the ffmpeg example they give in the documentation.) I don't know if that's still the case, I'll do some grepping through the core formulas and see.

@aome510
Copy link
Owner

aome510 commented May 21, 2023

https://github.com/Homebrew/homebrew-core/pull/128997/files#diff-9de770de980848d0bc612b5e6cb4910f1da3e81348b2dafbc877303fe477787dR21

IMO, it's better to install the application with default features. Not all non-default features work well without setting up or further configurations.

@aome510
Copy link
Owner

aome510 commented May 21, 2023

Also thanks for the PR in the core repo, look forward to seeing it getting merged.

@tresni
Copy link

tresni commented May 21, 2023

Okay, confirming that you are not allowed to use options in homebrew/core:

  * 8: col 3: Formulae in homebrew/core should not use `option`.
  * 26: col 27: Formulae in homebrew/core should not use `build.with?`.
  * 27: col 27: Formulae in homebrew/core should not use `build.with?`.
  * 28: col 34: Formulae in homebrew/core should not use `build.with?`.
  * 29: col 29: Formulae in homebrew/core should not use `build.with?`.

Using the ffmpeg formula as a guide, it would appear that enabling some optional features would generally be the recommendation. I would suggest that we leave the formula as is unless I'm missing something and there are cases where enabling images,lyric-finder,notify could actually cause the application to not work. In my (all be it) limited testing, I didn't find any issues with having them enabled if the underlying system didn't support the feature (e.g. running in Terminal vs iTerm with images just lead to block display, I haven't found a way to make lyric-finder not work, etc)

@aome510
Copy link
Owner

aome510 commented May 22, 2023

One case that lyric-finder doesn't work: #155. And image doesn't display a "nice-looking" image unless you use kitty or iTerm2. notify can be quite annoying if you don't really need one.

For me, I didn't find any problems with image and lyric-finder as my two additional features.

My point is that not all people need the features listed above and sometimes they may not work as expected. Using the default features provide a good and comfortable baseline. Users can always install the app with more features enabled if they want.

That said, the PR is still yours so feel free to leave as-is if that's what you think the best.

@aome510
Copy link
Owner

aome510 commented Jun 10, 2023

Close this as Homebrew/homebrew-core#128997 has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants