[Mesa-dev] [PATCH] meson: Add Haiku platform support v3

Fri Feb 16 22:07:26 UTC 2018

On 16 February 2018 at 19:08, Alexander von Gluck IV
<kallisti5 at unixzen.com> wrote:
> February 16, 2018 11:49 AM, "Emil Velikov" <emil.l.velikov at gmail.com> wrote:
>> Hi Alexander,
>>
>> Did you drop the ball on the autotools patches? I could re-spin them
>> but have no way to test.
>
> I've been focused on meson since it's the future.
> Feel free to re-spin them if you have the bandwidth.
>
>> There's a couple of comments inline, but nothing major.
>>
>> On 16 February 2018 at 00:32, Alexander von Gluck IV
>> <kallisti5 at unixzen.com> wrote:
>>
>>> --- a/meson.build
>>> +++ b/meson.build
>>>
>>> gl_priv_libs = []
>>> -if dep_thread.found()
>>> +if dep_thread.found() and host_machine.system() != 'haiku'
>>> gl_priv_libs += ['-lpthread', '-pthread']
>>> endif
>>
>> There's a bug report/fix in Meson for this one right?
>> Please mention the ticket number of Meson version where the fix
>> landed. Thus way one can drop this when we bump the Meson requirement.
>
> I did a bump of Meson on my Haiku build system and still saw the same
> incorrect searching for -lpthread issues.  I think there are places where
> -lpthread -pthread were specified without looking at dep_thread.found()
> though which could be at play.  (see below)
>
Precisely those are existing bugs and should be separate, preparatory patch.

>>> if dep_m.found()
>>> diff --git a/src/egl/meson.build b/src/egl/meson.build
>>> index 6cd04567b0..09c28e9f83 100644
>>> --- a/src/egl/meson.build
>>> +++ b/src/egl/meson.build
>>
>> There are three unrelated things happening here:
>>
>>> @@ -21,9 +21,8 @@
>>> c_args_for_egl = []
>>> link_for_egl = []
>>> deps_for_egl = []
>>> -incs_for_egl = [
>>> - inc_include, inc_src, inc_loader, inc_gbm, include_directories('main'),
>>> -]
>>> +incs_for_egl = [inc_include, inc_src, include_directories('main')]
>>> +
>>
>> a) inc_gbm should be moved in the with_platform_drm hunk
>
> Can-do.  I wasn't 100% sure on when it was included so didn't want to disrupt
> any non-Haiku build conditions.
>
It's an existing bug. Please flesh it out as separate commit.

>>> files_egl = files(
>>> 'main/eglapi.c',
>>> 'main/eglapi.h',
>>> @@ -53,9 +52,6 @@ files_egl = files(
>>> 'main/eglsync.h',
>>> 'main/eglentrypoint.h',
>>> 'main/egltypedefs.h',
>>> - 'drivers/dri2/egl_dri2.c',
>>> - 'drivers/dri2/egl_dri2.h',
>>> - 'drivers/dri2/egl_dri2_fallbacks.h',
>>> )
>>>
>>> linux_dmabuf_unstable_v1_protocol_c = custom_target(
>>> @@ -100,6 +96,21 @@ g_egldispatchstubs_h = custom_target(
>>> capture : true,
>>> )
>>>
>>> +if with_dri2
>>> + files_egl += files(
>>> + 'drivers/dri2/egl_dri2.c',
>>> + 'drivers/dri2/egl_dri2.h',
>>> + 'drivers/dri2/egl_dri2_fallbacks.h',
>>> + )
>>> + incs_for_egl += [inc_loader, inc_gbm]
>>> + c_args_for_egl += [
>>> + '-DDEFAULT_DRIVER_DIR="@0@"'.format(dri_search_path),
>>> + '-D_EGL_BUILT_IN_DRIVER_DRI2',
>>> + ]
>>> + link_for_egl += [libloader, libxmlconfig]
>>> + deps_for_egl += dep_libdrm
>>> +endif
>>> +
>>
>> b) This should be within the with_platform_x11 hunk
>
> So with_platform_x11 always means with_dri2 regardless
> of what with_dri2 is set to?
>
Errh, my bad. All of the whole
with_platforms_{x11,wayland,...everything-but-haiku} should be within
the with_dri2 block.

Alike the a) bit above, please keep that preparatory patch.

>>> if with_platform_x11
>>> files_egl += files('drivers/dri2/platform_x11.c')
>>> if with_dri3
>>> @@ -133,6 +144,15 @@ if with_platform_android
>>> deps_for_egl += dep_android
>>> files_egl += files('drivers/dri2/platform_android.c')
>>> endif
>>> +if with_platform_haiku
>>> + incs_for_egl += inc_haikugl
>>> + c_args_for_egl += [
>>> + '-D_EGL_BUILT_IN_DRIVER_HAIKU',
>>> + ]
>>> + files_egl += files('drivers/haiku/egl_haiku.cpp')
>>> + link_for_egl += libgl
>>> + deps_for_egl += cpp.find_library('be')
>>> +endif
>>
>> c) this is the patch introducing haiku support
>
> These are all changes needed to properly build.
>
>>> --- /dev/null
>>> +++ b/src/gallium/targets/haiku-softpipe/meson.build
>>>
>>> +libswpipe = shared_library(
>>>
>>> + dependencies : [
>>> + driver_swrast, cpp.find_library('be'), cpp.find_library('translation'),
>>> + cpp.find_library('network'), dep_unwind
>>
>> Some of these find_library calls are duplicated throughout. It would
>> be more efficient and robust to have it done once.
>> Say in the top level meson.build?
>
> That can be done.  Focused on functionality before optimizing in this first
> commit.
>

>>> index 8aaf58a623..7a4bcd3329 100644
>>> --- a/src/hgl/GLDispatcher.h
>>> +++ b/src/hgl/GLDispatcher.h
>>> @@ -15,7 +15,7 @@
>>> #include <GL/gl.h>
>>> #include <SupportDefs.h>
>>>
>>> -#include "glheader.h"
>>> +#include "main/glheader.h"
>>
>> Please keep source changes separate from build system bits.
>
> Can-do. This was a requirement and matches the other dispatchers like glx.
>
>>> diff --git a/src/mapi/es1api/meson.build b/src/mapi/es1api/meson.build
>>> index ea14654d2c..38a5747e9a 100644
>>> --- a/src/mapi/es1api/meson.build
>>> +++ b/src/mapi/es1api/meson.build
>>>
>>> --- a/src/mapi/es2api/meson.build
>>> +++ b/src/mapi/es2api/meson.build
>>> @@ -48,7 +48,7 @@ pkg.generate(
>>> description : 'Mesa OpenGL ES 2.0 library',
>>> version : meson.project_version(),
>>> libraries : libgles2,
>>> - libraries_private : '-lm -ldl -lpthread -pthread',
>>> + libraries_private : gl_priv_libs,
>>> )
>>
>> Unrelated fixes - make that a separate patch.
>>
>
> This is actually needed to compile EGL under Haiku since
> it is assuming -lpthread and -lm... so related.
>
These are preexisting bugs, I'm afraid. Please keep them a separate commit.

>>> diff --git a/src/meson.build b/src/meson.build
>>> index 730b2ff6e4..4d5637f0aa 100644
>>> --- a/src/meson.build
>>> +++ b/src/meson.build
>>>
>>> +if with_glx != 'disabled'
>>> + subdir('glx')
>>> +endif
>>
>> Another unrelated fix?
>
> If you try and compile glx on Haiku you're gonna have a bad
> day.  Setting glx to disabled should mean glx isn't built.
>
My meson isn't that bad - I can understand what the code does ;-)
I'm saying that this is, yet another preexisting bug. And hence patch.

Off the top of my head, the above bugs don't existing in the autotools build.
Which would explain why I suggested that one, as a stepping stone ;-)

>> Last but not least, please add a meson.build to the respective
>> Makefile.ams' EXTRA_DIST.
>
> Can-do.  I wasn't making any changes to Makefile / Autotools
> since the plan is to migrate away from it.
>
Pardon, I was too vague here. If the files are not in EXTRA_DIST
they'll be missing from the tarball.

Thanks
Emil