mpv icon indicating copy to clipboard operation
mpv copied to clipboard

Fix gbm.h/jpeglib.h detection outside of default search path

Open jbeich opened this issue 4 years ago • 9 comments

The following scenario maybe uncommon on Linux but BSDs separate base system and packages to avoid tainting build environment:

  1. Install libjpeg-turbo under /myprefix
  2. Make sure libjpeg.pc points to /myprefix/include
  3. Run waf configure
  4. Confirm JPEG support detection failed
  5. Check build/config.log
  6. Notice -I/myprefix/include is missing

When a project provides *.pc file(s) it expects consumers to consult them how to find headers, what flags to use, etc. instead of probing them manually. Only when there's none one can blame BSDs for crippling the default environment. Fortunately, all affected places have *.pc files, so we can drop an ugly wart in FreeBSD CI config.

jbeich avatar May 25 '20 01:05 jbeich

Not sure what happened to this. Can't we just require pkg-config in all cases?

ghost avatar Jun 09 '20 15:06 ghost

Can't we just require pkg-config in all cases?

libjpeg.pc is not provided by IJG libjpeg unlike libjpeg-turbo. shaderc is used by d3d11 on Windows where I'm not sure if pkg-config is usable.

gbm.pc maybe missing on non-Mesa implementations but neither nvidia-driver nor raspberrypi-userland support libgbm. No clue about Android vendors.

jbeich avatar Jun 09 '20 17:06 jbeich

I doubt that there's non-mesa GBM (if it is, it's going to be some sort of horrible ARM-Linux thing with non-upstreamed drivers, not a big loss). That leaves libjpeg as possibly the only thing that needs a non-pc check? I'm a bit hesitant with needing complicated checks. Also, the new build system is work in progress.

ghost avatar Jun 09 '20 21:06 ghost

Sorry, after checking IJG libjpeg upstream:

Version 9c  14-Jan-2018
-----------------------
...
Add libjpeg pkg-config file.  Thank to Mark Lavi, Vincent Torri,
Patrick McMunn, and Huw Davies for suggestion.

FreeBSD package of IJG jpeg haven't been updated since 2014-12-10 as libjpeg-turbo became default on 2015-09-16

jbeich avatar Jun 09 '20 22:06 jbeich

#8208 regressed Wayland support (only with dbc3ab38807c in mind) as <linux/input-event-codes.h> is now checked with default compiler flags but default only covers base system while the header is part of evdev-proto package on DragonFly and FreeBSD. <dev/evdev/input-event-codes.h> is not supposed to be used by portable software.

jbeich avatar Oct 25 '20 22:10 jbeich

OK, so I was for a bit confused since the CI was still passing, but it seems like that related change was in this PR indeed :) .

So the history of that PR is as follows:

  1. Someone on the mpv-devel IRC channel had netbsd installed, and noticed that it had Wayland stuff available, but compilation would then fail as linux/input.h was included.

  2. Thus the idea came to add a configure check for it, so that the lack of header would cause things already fail at the configure level, and not later in failing compilation.

  3. @linkmauve quoted the Wayland documentation as mentioning linux/input-event-codes.h as being the upstream of the event identifiers for Wayland instead of the more generic header. This was found to not break the existing CI flow, and work on linux as well (requires Linux 4.4 or so I think where the header was split?)

  4. Thus a PR was added which: a) changes the header b) adds a configure check (which would have been added in any case since someone noticed compilation was broken and apparently Wayland headers etc were available).

    The b) part of step 4 would have been most likely taken anyways, just to make Wayland not get enabled in NetBSD.

I would guesstimate that adding any configure check for the header would fail this, or is the new split header somehow special?

jeeb avatar Oct 25 '20 23:10 jeeb

adding any configure check for the header would fail this

Correct. Any header from packages that isn't backed by pkg-config --cflags.

I'm not sure how to fix this yet.

jbeich avatar Oct 25 '20 23:10 jbeich

1. Someone on the mpv-devel IRC channel had netbsd installed, and noticed that it had Wayland stuff available, but compilation would then fail as `linux/input.h` was included.

mpv package in PkgSrc does support Wayland on NetBSD. However, @niacat probably didn't upstream everything which messes up expectations for those who build outside of PkgSrc.

https://github.com/NetBSD/pkgsrc/blob/trunk/multimedia/mpv/patches/patch-video_out_wayland__common.c

jbeich avatar Oct 25 '20 23:10 jbeich

Yea, we had something very similar originally in the PR (redefinition of the utilized definitions), but unfortunately there were some conflicting reports on whether an actual Wayland compositor was available, so instead of adding extra ifdefs for "nothing" (since the person on mpv-devel wasn't sure how to test it, either - and FreeBSD seemed to provide the compatibility headers), it was just made to fail configure.

edit: I think the initial end result with regards to what would happen if we would add those re-definitions, those would probably go under a helper header under osdep/ instead. That way the actual wayland code and the re-definitions would be kept separate.

But since the Linux header is the effective upstream, we would totally prefer that to be provided if possible (if some system standardizes on the pkg-config name for Linux compatibility layers, we're 100% open for adding checks for that as far as I can tell - and hopefully for more than one non-Linux *nix system).

Alternatively this should be brought up again to the Wayland folks and it requested for definitions to be duplicated on systems that aren't Linux.

jeeb avatar Oct 26 '20 00:10 jeeb

Glancing at this again, the meson build actually does use pkg-config on all the mentioned libraries in here (gbm, jpeg, and shaderc) so this is solved really. The wayland thing is separate of course (and there's another PR for that). I'll go ahead and close this.

Dudemanguy avatar Feb 26 '23 23:02 Dudemanguy