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

Fixed build instructions in README.md, updated script help text #2455

Merged
merged 1 commit into from May 19, 2019

Conversation

3 participants
@athairus
Copy link
Contributor

commented May 12, 2019

Fixes:

  • README.md references libmpv/include/mpv/ when in reality it’s deps/include/mpv/
  • README.md doesn’t mention running pod install when you’re manually adding libraries
  • README.md has some misc typos
  • other/change_lib_dependencies.rb references the wrong Homebrew package in its help text

Workarounds:

  • Homebrew’s mpv-iina package produces libmpv.1.*.0.dylib without +w which breaks the script other/change_lib_dependencies.rb (Homebrew’s mpv package does have +w on that .dylib file), added a step in README.md to fix that

  • This change has been discussed with the author.

  • It implements / fixes issue #.


Description:
I've tried building IINA using the instructions in README.md and ran into several issues. This PR updates the instructions to better reflect the actual steps needed to build the project from scratch. I also fixed a few miscellaneous typos in README.md and other/change_lib_dependencies.rb and tweaked the instruction's flow a bit to make it more linear.

@alejx alejx requested review from saagarjha and alejx May 12, 2019

@saagarjha

This comment has been minimized.

Copy link
Member

commented May 12, 2019

I'm a bit busy this weekend, so I'll look at this in a day or two and merge it then. From a quick glance I think this is fine, though.

@saagarjha
Copy link
Member

left a comment

Thanks for the fixes! Agree with most of the changes; I'm glad to have someone who uses Homebrew run through the list as I was mostly writing them from memory and I didn't really test them…

Before we merge this, I have one question about the way Homebrew builds libmpv: would you mind explaining the issue in a little more detail, and whether my proposed solution would fix it for you?

Show resolved Hide resolved README.md Outdated
Show resolved Hide resolved other/change_lib_dependencies.rb
Fixed build instructions in README.md, updated script help text
Fixes:
- `README.md` references `libmpv/include/mpv/` when in reality it’s `deps/include/mpv/`
- `README.md` doesn’t mention running pod install when you’re manually adding libraries
- `README.md` has some misc typos
- `other/change_lib_dependencies.rb` references the wrong Homebrew package in its help text

@athairus athairus force-pushed the athairus:readme-fixes branch from b88affa to 03d1232 May 13, 2019

@alejx

alejx approved these changes May 14, 2019

Copy link
Member

left a comment

LGTM

@alejx alejx requested a review from lhc70000 May 14, 2019

@saagarjha saagarjha merged commit ab8a369 into iina:develop May 19, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@saagarjha

This comment has been minimized.

Copy link
Member

commented May 19, 2019

Thanks!

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