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.

Sign up for GitHub

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

Jump to bottom

mpv 0.30 #45854

Closed
wants to merge 1 commit into from
Closed

mpv 0.30 #45854

wants to merge 1 commit into from

Conversation

jnozsc
Copy link
Contributor

@jnozsc jnozsc commented Oct 26, 2019 •
edited
Loading

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@BrewTestBot
Copy link
Member

  • Dependency 'libarchive' is provided by macOS; please replace 'depends_on' with 'uses_from_macos'.

@jnozsc
Copy link
Contributor Author

jnozsc commented Oct 26, 2019 •
edited
Loading

I don't know why the @BrewTestBot suggest uses_from_macos: libarchive, but when I test on commit 0ddf90a

I get error message as https://jenkins.brew.sh/job/Homebrew%20Core%20Pull%20Requests/50536/version=catalina/testReport/junit/brew-test-bot/catalina/install___build_bottle_mpv/

Checking for libarchive wrapper for reading zip files and more            : no ('libarchive >= 3.0.0' not found) 
You manually enabled the feature 'libarchive', but the autodetection check failed.

I check the code from mpv https://github.com/mpv-player/mpv/blame/master/wscript#L416 , and I don't think mpv actually looking for the one @BrewTestBot or uses_from_macos points to.

is there any workaround to skip this warning or fix it? Thanks

@fxcoudert fxcoudert added the new formula PR adds a new formula to Homebrew/homebrew-core label Oct 27, 2019
@chenrui333
Copy link
Member

Looks like we have removed it per this PR, #44282?

@jnozsc
Copy link
Contributor Author

jnozsc commented Oct 28, 2019

yes, I believe mpv 0.29 does not support latest version of ffmpeg, which block the unit test for the PR to upgrade ffmpeg to 4.2.1, and it seems back to that time, before Catalina releases, mpv team do not think it is necessary to release a new version for mpv

but now, the mpv 0.30 fix the issue

fxcoudert
fxcoudert reviewed Oct 28, 2019
View reviewed changes
Formula/mpv.rb Outdated
depends_on "pkg-config" => :build
depends_on "python" => :build

depends_on "ffmpeg" => "4.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
depends_on "ffmpeg" => "4.0"
depends_on "ffmpeg"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't accept pinned versions of dependencies. Dependencies are “latest version”, always.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix in 589c060

Formula/mpv.rb Outdated

depends_on "ffmpeg" => "4.0"
depends_on "jpeg"
# uses_from_macos "libarchive" does not work
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove that comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix in 589c060

Formula/mpv.rb
depends_on "libarchive"
depends_on "libass"
depends_on "little-cms2"
depends_on "lua@5.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't build with latest lua version (5.3)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately, mpv does not support lua 5.3 at this point mpv-player/mpv#5205

@jnozsc jnozsc requested a review from fxcoudert October 30, 2019 03:48
@fxcoudert fxcoudert added the maintainer feedback Additional maintainers' opinions may be needed label Nov 4, 2019
@fxcoudert
Copy link
Member

@Homebrew/core opinion on this?

@SMillerDev
Copy link
Member

Seeing how bad the experience packaging mpv has been I'm inclined to say it should just be in a tap somewhere.

MikeMcQuaid
MikeMcQuaid reviewed Nov 4, 2019
View reviewed changes
Formula/mpv.rb Outdated
system "python3", "waf", "install"

system "python3", "TOOLS/osxbundle.py", "build/mpv"
prefix.install "build/mpv.app"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't install a .app for Homebrew/homebrew-core, thanks. That's what homebrew/cask is for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank @MikeMcQuaid , it seems that install as a .app was an option, then it became default behavior in this commit 1fdd9b1 do you want me to add the with-bundle option back?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No option, simply do not install the app. People who want the app can install from cask

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleted in 0c7e9af

@MikeMcQuaid
Copy link
Member

Once the .app is removed: I'm fine with including this. Ultimately if our users want it: we may as well provide it.

@fxcoudert
Copy link
Member

OK, we're good to go. Please squash all changes into a single commit (mpv 0.30) and we can merge!

@fxcoudert fxcoudert added ready to merge PR can be merged once CI is green and removed maintainer feedback Additional maintainers' opinions may be needed labels Nov 5, 2019
@MikeMcQuaid
Copy link
Member

Thanks so much again for your contribution! Without people like you submitting PRs we couldn't run this project. You rock, @jnozsc!

@retokromer retokromer mentioned this pull request Nov 5, 2019
Closed
@lock lock bot added the outdated PR was locked due to age label Jan 3, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fxcoudert fxcoudert fxcoudert approved these changes

@MikeMcQuaid MikeMcQuaid MikeMcQuaid left review comments

Assignees
No one assigned
Labels
new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age ready to merge PR can be merged once CI is green
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jnozsc @BrewTestBot @chenrui333 @fxcoudert @SMillerDev @MikeMcQuaid