mpv icon indicating copy to clipboard operation
mpv copied to clipboard

libmpv: use correct header directory name

Open kasper93 opened this issue 1 year ago • 5 comments

libmpv headers are installed to mpv/, so why are we pretending it is libmpv?

Fixes documentation, libmpv meson dependency, tests.

Read this before you submit this pull request: https://github.com/mpv-player/mpv/blob/master/DOCS/contribute.md

Reading this link and following the rules will get your pull request reviewed and merged faster. Nobody wants lazy pull requests.

kasper93 avatar Jul 06 '24 11:07 kasper93

My only slight opposition is that it's not immediately obvious what the directory mpv is for to someone not familiar with the codebase.

Dudemanguy avatar Jul 06 '24 14:07 Dudemanguy

I agree, but this imho is the most frictionless change to make libmpv_dep work correctly. Now it is impossible to reference the internal headers.

kasper93 avatar Jul 07 '24 00:07 kasper93

What happens if I have mpv/client.h installed to the system include path? Does this PR guarantee that when compling mpv, the client headers from the mpv source is used instead of the one from system include path (which can be from a wrong version)?

na-na-hi avatar Jul 07 '24 01:07 na-na-hi

What happens if I have mpv/client.h installed to the system include path? Does this PR guarantee that when compling mpv, the client headers from the mpv source is used instead of the one from system include path (which can be from a wrong version)?

Yes, it is Meson job to add -I in proper place, so that internal headers take precedence.

Although, I had problems with Meson in regards to subproject linking, where linker would pick installed version instead of internal one. Again, Meson responsibility.

kasper93 avatar Jul 07 '24 01:07 kasper93

For this to make sense, we probably should have any users that are interested in using mpv as a subproject. Maybe mpv-build or mpv-examples could test something like that. Can be revisited later.

kasper93 avatar Dec 11 '24 15:12 kasper93

I hit this issue again, when trying to link with libmpv.

Maybe mpv-build or mpv-examples could test something like that.

I was wrong here. We have multiple tests, that already fully test linking to dependency. After this commit of course, because before they were manually linked.

My only slight opposition is that it's not immediately obvious what the directory mpv is for to someone not familiar with the codebase.

Well, we install those headers to mpv, so it's even worse to understand from outside of the repository?

kasper93 avatar Feb 22 '25 05:02 kasper93

I tested it with external project too. @Dudemanguy @sfan5 do you have strong objection about this? I think it is correct way of doing things in meson and while historically meson was not alone, now it it, so we should support _dep and subprojects.

kasper93 avatar Feb 22 '25 08:02 kasper93

I was just unhappy with a top level mpv folder. include/mpv is good.

sfan5 avatar Feb 22 '25 10:02 sfan5

+1 for include/mpv I like that better than current as well

llyyr avatar Feb 22 '25 11:02 llyyr

include/mpv works for me too.

Dudemanguy avatar Feb 22 '25 16:02 Dudemanguy

Thank you.

kasper93 avatar Feb 23 '25 00:02 kasper93