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

README: add trick to better leverage mpv-build #7655

Closed
wants to merge 1 commit into from

Conversation

ahimta
Copy link
Contributor

@ahimta ahimta commented Apr 23, 2020

No description provided.

@Akemi
Copy link
Member

Akemi commented Apr 23, 2020

tbh this seems like something that shouldn't be part of the readme.

@avih
Copy link
Member

avih commented Apr 23, 2020

tbh this seems like something that shouldn't be part of the readme.

I think the same.

@ahimta
Copy link
Contributor Author

ahimta commented Apr 25, 2020

I think it would be super helpful for this to be everywhere where a build (especially for Linux) is mentioned (incl. the mpv-build). Because the alternative (i.e: completely building from source) is a blocker for beginners and takes unnecessary time from non-beginners.

This is only a mitigation for the build for contributors (existing and especially new). If it weren't mentioned alongside the build or presented as the recommended approach very few will know of its existence. Beginners wouldn't know to look and non-beginners would probably dismiss it even if they found it in a separate place because they would most likely assume it to be too good to be true (which may be a good heuristic that pays off most of the time).

I'm on a fringe where I think the build requirements for most projects (open source and otherwise) are a huge source of accidental complexity and am planning to publish some work that would address this in the coming months and hope to apply it to mpv and other projects that interest me.

@ghost
Copy link

ghost commented Apr 25, 2020

Would it make sense to put this into the mpv-build README?
Also not sure how safe this "trick" is.

@ahimta
Copy link
Contributor Author

ahimta commented Apr 25, 2020

I think it would be best if this is the recommended approach on Linux and be presented as such everywhere (mpv, mpv-build). This would naturally require me to put more effort to achieve this and it would be my pleasure.

The existing/conventional approach would be presented as a fallback. I would also make sure the trick is tested in the CI system on different Linux distros. I did something similar with PCSX2 which is harder due to mutilib-stuff (QJoyPad too).

I will do my best so the final result meets your expectations.

And as I mentioned before, this is all a mitigation and I'm working on a solution that works for most projects because even for myself alone, the way building from source is for most projects is non-sustainable and I can't even work on most projects without spending a whole lot of work on building effort that is non-transferable to other projects.

I believe the build situation for most projects is acceptable maybe with respect to the standards 10 years ago but I think we can now do so much better today (with building steps/instructions that actually work). I just have to put a lot of effort to prove it or embarrass myself trying.

@avih
Copy link
Member

avih commented Apr 25, 2020

Do correct me if I'm wrong, but as far as I can tell the problem which this "tip" is supposed to help with is if you already have an mpv directory where you work on things of your own, or maybe have some patches applied, etc, and you want to use mpv-build to build your own source directory instead of the one which mpv-build clones.

Is that correct?

If yes, then I think the tip is bad because currently mpv-build assumes ownership of the mpv source dir, and it does not try to preserve prior content at that dir. It can modify the remote URL (e.g. if you cloned from your fork and not from the upstream repo), if can overwrite changes at your tree, etc.

And even if under some conditions (which, by the way, you didn't mention) it does work, it's not guaranteed to keep working if mpv-build itself is updated.

IMO it's just a bad, incorrect, and dangerous tip.

@ahimta
Copy link
Contributor Author

ahimta commented Apr 26, 2020

Excellent point @avih. You are correct. And I did use mpv-builder successfully with my fork before.

I don't see this as a blocker and if it were so easy it would have been done already. There will be other issues too.

mpv-builder is part of mpv-player and if this requires changes to mpv-builder so be it. It may even change the primary use of mpv-builder to support this use-case.

The way I look at it is that it may be possible to adapt mpv-builder and mpv to make build instructions work as is but it may be intractable to have build instructions that work with Linux otherwise. And it's often more efficient to solve a problem, as hard as it may be, than have everyone solve it for themselves separately.

What happens with the conventional approach most of the time is that you have 10 commands but they just don't work and you do some research and more commands than all platforms combined spending more time than your contribution.

I don't want to ramble about the issues of building from source but I will say that the biggest issue with such inconveniences is that once someone knows how to tackle them it becomes less important for them to make the inconvenience disappear altogether.

With that being said, especially with the problems @avih highlighted, I can see how this approach may be less appealing and more risky for the project. It also depends on whether the build or contributors for mpv is a bottleneck at the moment.

Finally, I will do whatever you choose since you work on the project more and know best and will come back later when I'm done with my initiative for solving this problem for different projects.

And please do close the issue if you see fit and thanks for your time!

@avih
Copy link
Member

avih commented Apr 26, 2020

FWIW, the tip is bad only for update and rebuild - which can overwrite the content etc. If you update the source-dir yourself and then only use build then it doesn't modify files inside the source tree (other than at the mpv/build/ directory which is not part of the repository).

Additionally, there has been some work to help with such cases and more with mpv-build, see this issue mpv-player/mpv-build#97 - it has a link to my fork which includes many enhancements (and which I personally still use to build and update mpv), but for now it's not merged upstream.

Bottom line, your tip is fine for build, but can be really bad and corrupt your data for anything else which mpv-build offers.

@kevmitch
Copy link
Member

I understand that building things can get a bit complicated sometimes, but I don't see how this universally simplifies things or solves any commonly hit problems. Everyone is going to have their preferred way of doing things and if we add a "tip" for each one, it's only going to become more confusing, not less.

Personally, I just work on mpv directly in the mpv-build directory.

@ahimta
Copy link
Contributor Author

ahimta commented Apr 26, 2020

@kevmitch your approach is definitely better and less convoluted than mine. I think I've said all there is to say in my previous reply and then some. mpv members know best.

@Akemi
Copy link
Member

Akemi commented Jun 27, 2020

i am closing this since it seems like the consensus is to not add it and to not leave this in limbo any longer.

@Akemi Akemi closed this Jun 27, 2020
Copy link

@momojackripad momojackripad left a comment

Choose a reason for hiding this comment

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

Copy link

@momojackripad momojackripad left a comment

Choose a reason for hiding this comment

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

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

Successfully merging this pull request may close these issues.

5 participants