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

Fix for issue 5004 not been pushed to brew #5080

Closed
weichuliu opened this issue Nov 5, 2017 · 24 comments
Closed

Fix for issue 5004 not been pushed to brew #5080

weichuliu opened this issue Nov 5, 2017 · 24 comments

Comments

@weichuliu
Copy link

mpv version and platform

(brew mpv) 0.27.0_1
macOX High Sierra

Reproduction steps

Double click video file 1, then double click video file 2.

Expected behavior

Existed mpv playing new video.

Actual behavior

mpv being not usable.

Hi @Akemi
I have seen you fixed the issue #5004 (77021cf) ,Thank you for fixing it.
However it is been 3 weeks but the fix is not yet pushed to brew formula.
As far as I can see the latest release in https://github.com/mpv-player/mpv/releases is Sep 13th.
I am surprised that the issue was handled and fixed promptly, however after 3 weeks I am still getting 3 mpv fake icons on my Dock.

Can mpv team do a new release in brew?

@Argon-
Copy link
Member

Argon- commented Nov 5, 2017

mpv has nothing to do with homebrew, it is entirely their responsibility what is available in their package manager.
In a normal world you could do a brew install mpv --HEAD build in brew and get a fixed version but this is currently broken.

@weichuliu
Copy link
Author

weichuliu commented Nov 5, 2017

@Argon-
Thank you for the info. I had the misunderstanding that brew formula was managed by this repo.
However I think the new formula still depends on new release from this repo as suggested from the formula content:
https://github.com/Homebrew/homebrew-core/blob/master/Formula/mpv.rb

Also, FYI the install from HEAD is not working, as is is throwing the following ERROR:
As you mentioned, currently building from HEAD throws the following message:

Unable to find development files for some of the required FFmpeg/Libav libraries. You need git master. For FFmpeg, the mpv fork, that might contain additional fixes and features is required. It is available on https://github.com/mpv-player/ffmpeg-mpv Aborting.

READ THIS: https://docs.brew.sh/Troubleshooting.html

These open issues may also help:
Mpv and ffmpeg  https://github.com/Homebrew/homebrew-core/issues/20104

@weichuliu
Copy link
Author

weichuliu commented Nov 5, 2017

Still, I wonder when will mpv cut a new version, because if I understand correctly, brew formula still depends on it.

@Argon-
Copy link
Member

Argon- commented Nov 5, 2017

New mpv releases usually come every 2-3 months but there is no real schedule.

However, the next release might take a while, considering mpv underwent a major change in its dependencies (requires a fork of ffmpeg now). Also see the last sentence here: 83d44ac#commitcomment-25386604

At the same time, the brew people need to figure out how to support this change in their brew formula. As of yet, no conclusion was reached on how to do that (see Homebrew/homebrew-core#20104).

@dreness
Copy link

dreness commented Nov 14, 2017

@weichuliu I worked around this by changing the git url in the ffmpeg formula to the url of the mpv fork of ffmpeg. Doing this implies being ok with using the mpv fork of ffmpeg all the time (ie even when using ffmpeg outside of mpv). I haven’t found any fallout, but that could change...

@Hrxn
Copy link
Contributor

Hrxn commented Nov 14, 2017

I don't understand why they don't use the approach of static mpv binary linked with ffmpeg-mpv, to be honest.
At the moment, this would seem like the most reasonable solution to me.

@Argon-
Copy link
Member

Argon- commented Nov 22, 2017

@Hrxn I don't really get that either, this would certainly be the "right" approach for mpv.
Problem is, this either requires duplicating homebrew's ffmpeg formula or directly compiling ffmpeg-mpv inside the mpv formula. The latter would somehow need a way to allow users to specify config options. This is not a technical but rather a UX problem.
There is a certain force trying to reduce (or not further increase) options in brew formulas and honestly, it completely goes beyond me why. There have actively been thoughts about reducing the numbers of options provided in the mpv formula (which apparently has the highest number of options in all of brew) to make it look nicer. This is ridiculous imo...

@AirPort
Copy link

AirPort commented Nov 22, 2017

I honestly don't get it either: I mean, the whole point of installing from source instead of using the precompiled bottle is to be able to customize options. I guess limiting them makes sense for obscure/abandoned formulas that may break at any given time with options changed or removed from HEAD, but that's definitely not the case for ffmpeg or mpv.

On the other hand I really don't suggest changing the ffmpeg url in the brew formula as it may lead to unexpected breakage, instead I suggest using the mpv-build scripts: I found out they work quite well on MacOS too with few to no modifications (depending on the ffmpeg/mpv custom options passed to the configure scripts).

@Akemi
Copy link
Member

Akemi commented Dec 8, 2017

building from HEAD should work again since the ffmpeg-mpv dependency was removed. though currently there is a problem with lua, mpv and homebrew i believe(?). they currently use lua 5.3 that is not compatible with mpv.

anyway i will close this since the actual issue has been fixed and this is a homebrew problem.

@Akemi Akemi closed this as completed Dec 8, 2017
@Argon-
Copy link
Member

Argon- commented Dec 8, 2017

It's trivial to fix this as brew has a frozen lua 5.1 formula they could use.

@CounterPillow
Copy link
Contributor

@Argon- too bad they've already completely refused to do that, and started deleting comments that argued against their view.

@Argon-
Copy link
Member

Argon- commented Dec 8, 2017

I just found what you're probably referring to: Homebrew/homebrew-core#21389
Is this a joke? Are they aware that their package manager is -- right now -- incapable of building mpv with lua, which is basically a mandatory requirement at this point and far more then "recommended"?
And they could fix this with a change in one single line, yet they prefer... not to do so...?
In fact, I had this change (depend on lua@5.1 instead of lua) for years applied locally (even back when the formula was maintained by mpv).

Maybe it's necessary to bring back mpv's homebrew formula under control of mpv maintainers. This is not the first time where the formula is in a near-unusable state for an extended period of time, simply for philosophical reasons.
I also heard they once thought (or still think?) about reducing the options a user has simply because of "there shouldn't be so many options".

@CounterPillow
Copy link
Contributor

Maybe it's necessary to bring back mpv's homebrew formula under control of mpv maintainers.

I don't use macOS (I was mostly interested in clearing up their mistake of thinking using Lua 5.3 was on the horizon or easy), but if you think this would reduce the number of "halp can't ytdl" and "omg where did osc go???" issues and want to put in the work, I think it might be worthwhile.

mpv already works around Linux distro brainfarts with mpv-build, so if homebrew wants to approach Debian-levels of brain damage the same treatment may be in order. Their hostile stance towards dissenting opinions makes it unlikely they'll change their mind.

@ilovezfs
Copy link

ilovezfs commented Dec 9, 2017

It's trivial to fix this as brew has a frozen lua 5.1 formula they could use.

It's not trivial in that the 5.1 formula will eventually be removed. The sooner you can depend on something other than an @ formula, the better. luajit may be an option.

Maybe it's necessary to bring back mpv's homebrew formula under control of mpv maintainers.

We will not be removing the mpv formula under just about any circumstances, so that would not be worth doing as it's not actually possible. The formula has had about 8,000 in the last 30 days and we reject removals routinely for things with far less than that.

too bad they've already completely refused to do that

I haven't actually refused to do anything. It's important, however, that upstream be aware that the depends_on lua@5.1 cannot be a long-term solution.

I have merged the 5.1 PR. Let me know if you need me to do anything else and sorry for the trouble. I didn't intend to break mpv and thought that it did build successfully with lua 5.3 as the build did not fail and the test block passed. Maybe there is a configure option we can add to the formula to force configure-time failure instead of silently building without lua? Or at least we could improve the test block so that it fails if lua support isn't working. I would accept PR(s) for that.

I guess limiting them makes sense for obscure/abandoned formulas that may break at any given time with options changed or removed from HEAD, but that's definitely not the case for ffmpeg or mpv.

In the case of mpv, it's been proposed, by the same person, two times, and I have rejected that proposal both times.

@ilovezfs
Copy link

ilovezfs commented Dec 9, 2017

I've opened #5205 to track this.

@Argon-
Copy link
Member

Argon- commented Dec 9, 2017

thought that it did build successfully with lua 5.3 as the build did not fail

The build system auto detects features. It looks for dependencies and enables the respective feature when it's found. When you make 5.3 visible in the formula's env lua support will still not be activated as the required dependencies are not found (5.1, 5.2, luajit).
It might be possible to change this by explicitly adding --enable-x parameters for every optional dependency (like it is done here: https://github.com/Homebrew/homebrew-core/blob/master/Formula/mpv.rb#L76 ※). In this case configure actively fails when the corresponding dependency was not found.
I'm not very familiar with waf and the build-system though but I think it might work like that.

※ I'm wondering why this is even done here but only for a few single options. I just checked the old formula and there was no such thing (https://github.com/mpv-player/homebrew-mpv/blob/d6fd634b95c068d43c0116f783c9a0cf6ae7db15/Formula/mpv.rb)

and sorry for the trouble

Please know that I'm not blaming you or other brew people for breakages like this. This stuff happens.
In this case the (temporary) solution was tremendously easy (essentially changing one single line), yet a lot of text was written for basically philosophical reasons.

We will not be removing the mpv formula under just about any circumstances

Well, that's okay. But at the end of the day such a local tap made it possible to bring fixes on very short notice to users. You can point the user to something instead of reiterating "please report to brew".

@ilovezfs
Copy link

ilovezfs commented Dec 9, 2017

You can point the user to something instead of reiterating "please report to brew".

Except, we do want them to report it to us …

@ilovezfs
Copy link

ilovezfs commented Dec 9, 2017

In this case the (temporary) solution was tremendously easy (essentially changing one single line), yet a lot of text was written for basically philosophical reasons.

The text was about someone needing to open #5205. "Unfortunately" that someone ended up having to be me this time, which doesn't scale well: https://github.com/Homebrew/homebrew-core/graphs/contributors?from=2017-01-01&to=2017-12-08&type=d

@Argon-
Copy link
Member

Argon- commented Dec 9, 2017

Well, of course, I probably should've added the implied "and wait until it's fixed". ;) And that's the best case. Back during the weeks of ffmpeg-mpv all you could tell to users was "yes we know it doesn't work, yes they know it doesn't work, no it is not going to be fixed".

"Unfortunately" that someone ended up having to be me this time

I don't want to be pessimistic but mpv never had a stance of "no 5.3!". In the last 3 years there have been 2 PRs and they all suffered from severe technical deficiencies. This was the only reason they were not merged. Therefore, I have to assume nobody (with the necessary skill) is interested in such a change. So... personally I wouldn't bet on something happening to be honest... :/

I also don't really approve of this process. So apparently there's some regulation within homebrew that mandates feature requests to be opened in upstream project X when X wants homebrew to keep their formulas working?
Isn't this essentially blackmailing...? "Open this ticket or we are not fixing your formula"? Yeah, sorry, no...

@ilovezfs
Copy link

ilovezfs commented Dec 9, 2017

It's about making sure the issue does actually get created, not blackmail. There's rarely much pushback or delay with that particular step in the process.

@ilovezfs
Copy link

ilovezfs commented Dec 9, 2017

It might be possible to change this by explicitly adding --enable-x parameters for every optional dependency

It looks like --enable-lua works.

@gbstan
Copy link

gbstan commented Dec 9, 2017

Thank you so much all at brew for your help with this. I understand this is not easy with different development teams priorities.

@ilovezfs
Copy link

ilovezfs commented Dec 9, 2017

@gbstan you're welcome!

@Argon- I added --enable-lua here Homebrew/homebrew-core#21498

@Argon-
Copy link
Member

Argon- commented Dec 11, 2017

@ilovezfs nice, this will certainly help in case something goes wrong in the future.
I'm not really familiar with brew formulas but is it possible to iterate over all passed --enable flags? One could then generate explicit --enable flags for waf for all given flags. But not sure if that's worth the effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants