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

ffmpeg 4.2 #42827

Open
wants to merge 2 commits into
base: master
from

Conversation

@chenrui333
Copy link
Contributor

commented Aug 5, 2019

Created with brew bump-formula-pr.

@lembacon

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Error: 1 problem in 1 formula detected
ffmpeg:
  * Non-executables were installed to "/usr/local/opt/ffmpeg/bin"
    The offending files are:
      /usr/local/opt/ffmpeg/bin/python
@robertleeblairjr

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

I believe the failure is the python binding to mpv when it's built/compiled. You could submit a bug report to mpv on GH.
https://github.com/mpv-player/mpv/issues

@retokromer

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

According to @lachs0r (mpv):

Not our fault.

@robertleeblairjr

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

According to @lachs0r (mpv):

Not our fault.

According to mpv compile page, they don't support linking against any version of FFmpeg other than the one they're currently using in the project, which is 4.1.x. So, this won't be resolved until mpv updates the project with using FFmpeg 4.2. However, I believe there is going to be a problem with having the commit because FFmpeg 4.2 won't be in the repo but the new version of mpv which requires it will want to build against it.

Copy/paste from mpv project page.

FFmpeg ABI compatibility
mpv does not support linking against FFmpeg versions it was not built with, even if the linked version is supposedly ABI-compatible with the version it was compiled against. Expect malfunctions, crashes, and security issues if you do it anyway.
The reason for not supporting this is because it creates far too much complexity with little to no benefit, coupled with absurd and unusable FFmpeg API artifacts.
Newer mpv versions will refuse to start if runtime and compile time FFmpeg library versions mismatch.

@robertleeblairjr

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

The homebrew formula for 'mpv' is pulling an archive of source that's not been updated since October 2018. If homebrew is going to build 'mpv' formula, then it would be better to do a build against mpv project's master branch which has updates to 'waf' whereby it has patches from the version included in the *.tar.gz source file this formula is downloading. I believe the problem would be resolved if this homebrew formula pulled the branch, which may not be possible, instead of an outdated archive on the mpv GitHub page.

@robertleeblairjr

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

If homebrew is going to use outdated project compiled source files, then they need to have something in place which will not keep requirement/dependency formula from not being updated by those formula which depend on it. In this situation, mpv project is claiming not their fault because master branch builds just fine with FFmpeg 4.2, along with python and other build requirements. The project doesn't provide source archived releases very often and there have been close to 400 commits/fixes since the 0.29.1 package on master. I would submit a PR for updating the mpv formula to git pull master for the mpv project but this goes against their Acceptable Formula stipulation. Although, they've made an exception for certain formula such as 'pip'. I believe they could make an exception for mpv so that FFmpeg, which should take precedence, can not conflict with it.
@SMillerDev Can you take a look at if mpv formula can be an exception for pulling master branch since the mpv project rarely updates their releases for source? It's conflicting with FFmpeg formula being updated. Thanks.

@SMillerDev

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

I would be very hesitant to make exceptions here. Can you at least request that they tag a release?

@robertleeblairjr

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

I would be very hesitant to make exceptions here. Can you at least request that they tag a release?

Went to mpv project to file an issue to see if they would update. @retokromer was on it. Thanks for the reply.

@SMillerDev

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Got a link?

EDIT: And what does ffmpeg suggest their packages link to? Just mpv master?

@robertleeblairjr

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

@SMillerDev Can you look at making an exception or just having mpv removed as formula and available only as a cask? I believe the mpv project, at the moment, is not going to update their snapshot releases which this formula relies on. So, who knows if another minor revision will be released, not likely, or when the major version will be released. A maintainer at the project stated 0.30 release is unknown. From the conversation, it's most likely not any time soon. I would think that either mpv would need to be removed or something changed so as not to conflict with FFmpeg which is used in other formula, as well. Perhaps, moving to just pulling from Master, which I realize is not optimal to the project's policy. However, those interested in mpv could get it from another tap, if they're wanting to build it, as well. Thoughts?

@robertleeblairjr

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

@robertleeblairjr

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

EDIT: And what does ffmpeg suggest their packages link to? Just mpv master?

I would have to do a little research to determine if the FFmpeg project has recommendations for mpv specifically. However, I wouldn't think there's a preference since mpv is dependent upon FFmpeg and not vice versa. Here's a list of projects which compile with needing FFmpeg.
https://trac.ffmpeg.org/wiki/Projects
The mpv formula doesn't take precedence over the FFmpeg formula though which is used and a requirement in other Homebrew-Core formula.

@robertleeblairjr

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

Got a link?

EDIT: And what does ffmpeg suggest their packages link to? Just mpv master?

Maintainers in this issue suggested using the cask if mpv will not build against FFmpeg 4.2. Unless, homebrew can make an exception to allow pulling master, instead of the much older source tarball, then this will stay broken for some time. As of right now, the two major choices to allow the new version of FFmpeg formula is to make the exception for mpv formula or remove it since there is a cask. Any other ideas?
https://github.com/mpv-player/mpv/issues/6850

@pigoz

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

mpv builds fine with FFmpeg 4.2, but in general if you build mpv with any version of FFmpeg, it is ABI compatible only with that version (which can cause crashes if you try to use dylibs from another FFmpeg version).

I suggest you use a revision to force a rebuild of the formula against the new FFmpeg. That's what other package maintainers do.

@robertleeblairjr

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

mpv builds fine with FFmpeg 4.2, but in general if you build mpv with any version of FFmpeg, it is ABI compatible only with that version (which can cause crashes if you try to use dylibs from another FFmpeg version).

I suggest you use a revision to force a rebuild of the formula against the new FFmpeg. That's what other package maintainers do.

Sounds good and I've learned something new. However it sounds like a catch-22 to me. How do you have FFmpeg with it's linked libraries committed to the repo in order to revise mpv so that it will build against the newly committed FFmpeg if it can't be committed because of failing a link test with mpv? Or, how do you have mpv rebuild whenever an FFmpeg PR needs checked/tested for commit?

@pigoz

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

You could add a separate commit in the same PR that bumps the revision. I'm not familiar that much with homebrew, but I would imagine whatever tests run, they do on the changeset and not the single commits.

@robertleeblairjr

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

You could add a separate commit in the same PR that bumps the revision. I'm not familiar that much with homebrew, but I would imagine whatever tests run, they do on the changeset and not the single commits.

Neither am I. But, that's a great suggestion if it's feasible. I'll see what I come across as to if this is either implemented in other formula and is doable.

@robertleeblairjr

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

Created with brew bump-formula-pr.

Can you close/reopen to initiate again or comment to @BrewTestBot ? I've submitted a PR for mpv to bump the revision from 3 > 4. Unsure if that's all which was needed in order to get this PR to pass mpv link test. But, giving it a shot.

@jonchang

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

A separate pull request to bump the revision of mpv is not needed, since it will still be building against ffmpeg 4.1.x. The revision bump should instead happen in this pull request so that ffmpeg 4.2 + mpv revision are built and tested at the same time.

Furthermore ffmpeg 4.2 is failing brew audit so none of that will happen until the audit is fixed.

@chenrui333

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Sure, restart a build now.

@chenrui333

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

@BrewTestBot test this please

@robertleeblairjr

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

A separate pull request to bump the revision of mpv is not needed, since it will still be building against ffmpeg 4.1.x. The revision bump should instead happen in this pull request so that ffmpeg 4.2 + mpv revision are built and tested at the same time.

Furthermore ffmpeg 4.2 is failing brew audit so none of that will happen until the audit is fixed.

Thanks. Yep, I can't do that in this PR. Also, didn't know it was failing because of audit. Thought it was just a link test fail with mpv and caused by python.
FFmpeg Dependencies from brew deps --installed --tree

ffmpeg
|── aom
|── fontconfig
│ └── freetype
│ └── libpng
|── freetype
│ └── libpng
|── frei0r
|── gnutls
│ |── gmp
│ |── libidn2
│ │ |── gettext
│ │ └── libunistring
│ |── libtasn1
│ |── libunistring
│ |── nettle
│ │ └── gmp
│ |── p11-kit
│ │ └── libffi
│ └── unbound
│ |── libevent
│ │ └── openssl
│ └── openssl
|── lame
|── libass
│ |── freetype
│ │ └── libpng
│ |── fribidi
│ └── harfbuzz
│ |── cairo
│ │ |── fontconfig
│ │ │ └── freetype
│ │ │ └── libpng
│ │ |── freetype
│ │ │ └── libpng
│ │ |── glib
│ │ │ |── gettext
│ │ │ |── libffi
│ │ │ |── pcre
│ │ │ └── python
│ │ │ |── gdbm
│ │ │ |── openssl
│ │ │ |── readline
│ │ │ |── sqlite
│ │ │ │ └── readline
│ │ │ └── xz
│ │ |── libpng
│ │ |── lzo
│ │ └── pixman
│ |── freetype
│ │ └── libpng
│ |── glib
│ │ |── gettext
│ │ |── libffi
│ │ |── pcre
│ │ └── python
│ │ |── gdbm
│ │ |── openssl
│ │ |── readline
│ │ |── sqlite
│ │ │ └── readline
│ │ └── xz
│ |── graphite2
│ └── icu4c
|── libbluray
│ |── fontconfig
│ │ └── freetype
│ │ └── libpng
│ └── freetype
│ └── libpng
|── libsoxr
|── libvorbis
│ └── libogg
|── libvpx
|── opencore-amr
|── openjpeg
│ |── libpng
│ |── libtiff
│ │ └── jpeg
│ └── little-cms2
│ |── jpeg
│ └── libtiff
│ └── jpeg
|── opus
|── rtmpdump
│ └── openssl
|── rubberband
│ |── libsamplerate
│ │ └── libsndfile
│ │ |── flac
│ │ │ └── libogg
│ │ |── libogg
│ │ └── libvorbis
│ │ └── libogg
│ └── libsndfile
│ |── flac
│ │ └── libogg
│ |── libogg
│ └── libvorbis
│ └── libogg
|── sdl2
|── snappy
|── speex
│ └── libogg
|── tesseract
│ |── leptonica
│ │ |── giflib
│ │ |── jpeg
│ │ |── libpng
│ │ |── libtiff
│ │ │ └── jpeg
│ │ |── openjpeg
│ │ │ |── libpng
│ │ │ |── libtiff
│ │ │ │ └── jpeg
│ │ │ └── little-cms2
│ │ │ |── jpeg
│ │ │ └── libtiff
│ │ │ └── jpeg
│ │ └── webp
│ │ |── jpeg
│ │ |── libpng
│ │ └── libtiff
│ │ └── jpeg
│ └── libtiff
│ └── jpeg
|── theora
│ |── libogg
│ └── libvorbis
│ └── libogg
|── x264
|── x265
|── xvid
└── xz

@jonchang

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Feel free to open a separate pull request if you can fix the audit and reverse dependency issues.

@robertleeblairjr

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

Feel free to open a separate pull request if you can fix the audit and reverse dependency issues.

Merely a novice hobbyist trying to help solve problems. So, my input is limited but I may be able to find more simple fixes. Looks like there are two libraries which build using python in ffmpeg; libass and glib. So, I wouldn't know what has changed to cause this issue from the last 4.1.4 > 4.2. Obviously, if these same dependencies which are used by the ffmpeg project had failed, then they would not have released this version. What does audit explicitly do which isn't allowed but is fine with building from source?

@jonchang

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Audit performs automated checks to ensure that submitted formula comply with various Homebrew policies. If you applied this pull request to your local install and ran brew install -s ffmpeg && brew audit ffmpeg you would likely get the same result as our CI infrastructure.

@robertleeblairjr

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

The audit installs gem bundler for dependencies. So, if python being installed by a library wasn't a problem before, then why would it be now? Would a dependency of the library being built need to be stated for the formula as just an option using :build?

@robertleeblairjr

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

Audit performs automated checks to ensure that submitted formula comply with various Homebrew policies. If you applied this pull request to your local install and ran brew install -s ffmpeg && brew audit ffmpeg you would likely get the same result as our CI infrastructure.

Alright, I'll look at if anything has changed/added by ffmpeg project for building. Is it possible that something that was changed from 4.1.4 > 4.2 is fine for building from source but now has caused audit to fail?

@jonchang

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

The audit installs gem bundler for dependencies. So, if python being installed by a library wasn't a problem before, then why would it be now? Would a dependency of the library being built need to be stated for the formula as just an option using :build?

I don't understand what this means, or how bundler is related to this audit message at all.

Error: 1 problem in 1 formula detected
ffmpeg:
  * Non-executables were installed to "/usr/local/opt/ffmpeg/bin"
    The offending files are:
      /usr/local/opt/ffmpeg/bin/python

This formula is installing a non-executable file called python into the bin folder. Therefore, the solution should involve not placing non-executables into the bin folder.

@robertleeblairjr

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

The audit installs gem bundler for dependencies. So, if python being installed by a library wasn't a problem before, then why would it be now? Would a dependency of the library being built need to be stated for the formula as just an option using :build?

I don't understand what this means, or how bundler is related to this audit message at all.

Error: 1 problem in 1 formula detected
ffmpeg:
  * Non-executables were installed to "/usr/local/opt/ffmpeg/bin"
    The offending files are:
      /usr/local/opt/ffmpeg/bin/python

This formula is installing a non-executable file called python into the bin folder. Therefore, the solution should involve not placing non-executables into the bin folder.

Yes, I get that. Should I assume this was not happening in 4.1.4 and this is only caused by ffmpeg? That was why I was asking. I don't know all of the details about audit and what's allowed and isn't. Perhaps, this is acceptable for just building from source but not when brew checks for dependencies and this one isn't declared? Maybe it needs to be declared as an option such as :build or :test are used for other formula?

@jonchang

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Yes, I get that. Should I assume this was not happening in 4.1.4 and this is only caused by ffmpeg? That was why I was asking.

I don't know. You can test this out yourself by running brew install -s and brew audit.

I don't know all of the details about audit and what's allowed and isn't.

brew audit will tell you what is allowed and what isn't.

Perhaps, this is acceptable for just building from source but not when brew checks for dependencies and this one isn't declared?

Formula which fail brew audit are generally not allowed in homebrew-core, so no, this wouldn't be acceptable as-is. See https://docs.brew.sh/How-To-Open-a-Homebrew-Pull-Request

Maybe it needs to be declared as an option such as :build or :test are used for other formula?

I don't know what "it" is referring to in this sentence.

As a general note - I'm going to need you to start actually working on this yourself. I'm willing to help you through this process but I'm going to need to see code or a pull request before responding to further questions about this.

@robertleeblairjr

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

Yes, I get that. Should I assume this was not happening in 4.1.4 and this is only caused by ffmpeg? That was why I was asking.

I don't know. You can test this out yourself by running brew install -s and brew audit.

I don't know all of the details about audit and what's allowed and isn't.

brew audit will tell you what is allowed and what isn't.

Perhaps, this is acceptable for just building from source but not when brew checks for dependencies and this one isn't declared?

Formula which fail brew audit are generally not allowed in homebrew-core, so no, this wouldn't be acceptable as-is. See https://docs.brew.sh/How-To-Open-a-Homebrew-Pull-Request

Maybe it needs to be declared as an option such as :build or :test are used for other formula?

I don't know what "it" is referring to in this sentence.

As a general note - I'm going to need you to start actually working on this yourself. I'm willing to help you through this process but I'm going to need to see code or a pull request before responding to further questions about this.

Yes, I get that. Should I assume this was not happening in 4.1.4 and this is only caused by ffmpeg? That was why I was asking.

I don't know. You can test this out yourself by running brew install -s and brew audit.

I don't know all of the details about audit and what's allowed and isn't.

brew audit will tell you what is allowed and what isn't.

Perhaps, this is acceptable for just building from source but not when brew checks for dependencies and this one isn't declared?

Formula which fail brew audit are generally not allowed in homebrew-core, so no, this wouldn't be acceptable as-is. See https://docs.brew.sh/How-To-Open-a-Homebrew-Pull-Request

Maybe it needs to be declared as an option such as :build or :test are used for other formula?

I don't know what "it" is referring to in this sentence.
The binary being installed, which references python, that's used during the check.

As a general note - I'm going to need you to start actually working on this yourself. I'm willing to help you through this process but I'm going to need to see code or a pull request before responding to further questions about this.

You're talking to someone, like many people who use homebrew, who know little to no "coding" of formula they use to include the project. To top it off, if their primary occupation isn't even related, then this doesn't help the foundation of their knowledge to approach or solve problems the same as a dev or other person who's daily activities are centered around software. So, I'm guessing this will stay a PR for quite some time unless it's an actual simple remedy that someone spots, FFmpeg project has another update which "corrects" this, or some other difference between between the current 4.1.4 > 4.2 that's caused audit to fail. In my free time, I'll give it a go and see what else I could contribute.

@retokromer retokromer referenced this pull request Aug 9, 2019
5 of 5 tasks complete
@retokromer

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2019

@chenrui333 Could you possibly add the bump revision of mpv commit here in this PR?

@chenrui333

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

Sure, doing that now.

@chenrui333

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

Still failing:

16:09:36 /private/tmp/mpv-20190810-80715-1gwap3d/mpv-0.29.1/video/out/cocoa_cb_common.swift:337:39: error: cannot call value of non-function type '[NSScreen]'
16:09:36         let screens = NSScreen.screens()
16:09:36                       ~~~~~~~~~~~~~~~~^~
16:09:36                                       
16:09:36 
16:09:36 Waf: Leaving directory `/private/tmp/mpv-20190810-80715-1gwap3d/mpv-0.29.1/build'
16:09:36 Build failed
16:09:36  -> task in 'osdep/macOS_swift.o osdep/macOS_swift.h osdep/macOS_swift.swiftmodule' failed with exit status 1 (run with -v to display more information)
16:09:36 
@retokromer

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

The “python” issue can be resolved either upstream by avoiding the additional python folder created in http://git.ffmpeg.org/gitweb/ffmpeg.git/commit/50e194e6e126b7fe8e91ecc2e929335d4dc64fcf or here by modifying:

bin.install Dir["tools/*"].select { |f| File.executable? f }

in order to install convert.py and convert_from_tensorflow.py directly on the bin level rather than into bin/python.

@JCount

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

The v0.29.1 release of mpv will not build using any Xcode release after 10.1. This is due to the removal of support for Swift 3 in the 10.2 release.

See: Swift 5 Release Notes for Xcode 10.2

Swift 3 mode has been removed. Supported values for the -swift-version flag are
4, 4.2, and 5. (43101816)

The code has been migrated to support Swift 5 with Swift 4 as a fallback, as well as dynamic linking of the libs.

However, these changes currently only exist on the master branch of mpv and are not included in any tags. The head spec builds and works fine for me on my Mac running 10.14.16, after being built with Xcode 10.3 build 10G8.

This puts the maintainers in a bit of a sticky situation, when a dependency breaks without an easy fix but that is par for the course.

One less than ideal solution that has limited precedent would be simply distribute the bottle that is generated for High Sierra to Mojave as well. This would probably require some manual testing, but unless things have changed significantly since I was a maintainer, it should work.

Whether you want to go there is another matter entirely, and one I no longer have to spend time considering. 😄

@retokromer

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

However, these changes currently only exist on the master branch of mpv and are not included in any tags.

The mpv maintainers were asked to make a new release/tag.

@JCount

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

@retokromer Yes, but they do not plan on doing that as far as I can tell, which means that a creative solution is needed in order for this PR to go through, given that this is an Xcode compatibility issue. In terms of a new tag, they did not seem to be very responsive to the idea, and generally haven't been in the past. You saw that philosophy yourself in the issue you opened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.