Re: [Mesa-dev] [PATCH 03/18] glx: meson: build src/glx only with -Dglx=dri
On Tue, 18 Dec 2018 at 21:52, Dylan Baker wrote: > > Quoting Emil Velikov (2018-12-18 02:33:56) > > On Mon, 17 Dec 2018 at 19:44, Dylan Baker wrote: > > > > > > Quoting Emil Velikov (2018-12-17 10:58:11) > > > > On Fri, 14 Dec 2018 at 17:13, Dylan Baker wrote: > > > > > > > > > > Quoting Emil Velikov (2018-12-13 08:05:52) > > > > > > From: Emil Velikov > > > > > > > > > > > > The library is the dri capable one, push the check src/meson.build, > > > > > > instead of the current partial handling in src/glx/meson.build. > > > > > > > > > > > > Fixes: a47c525f328 ("meson: build glx") > > > > > > > > > > This is just a refactor, the Fixes: is unnecessary. More on that below > > > > > > > > > If I'm reading things correctly, building with -Dglx=xlib/gallium-xlib > > > > -Dshared-glapi -Denable-tests - will attempt to build the tests in > > > > src/glx/tests/. > > > > Which is something that shouldn't happen IMHO. > > > > > > Wait, why shouldn't they run? The tests pass (at least with the > > > gallium-xlib > > > glx), and running tests seems useful. > > > > > Fully agree running tests is useful and welcome. > > > > The tests flex the -Dglx=dri code in src/glx/. As-is it's rather > > confusing, misleading even, to request one thing then run tests for > > something that looks the same but isn't. > > Silly analogy - running ANV tests, when -Dvulkan-drivers=amd is used. > > > > Hope this is clear - not sure if the coffee has kicked in ;-) > > -Emil > > Sorry, I meant to respond yesterday. Ian explained why these tests are only > useful if building DRI based GLX. > > Maybe make the commit message something like: > "meson: Ensure tests for dri based glx are only built when dri based glx is > enabled" > or something like that? I'll leave it up to you what you think is best. As > long > as you note that the real functional change is not building tests when we > don't > want/need them. > > Reviewed-by: Dylan Baker Ack will do. Thanks. -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 03/18] glx: meson: build src/glx only with -Dglx=dri
Quoting Emil Velikov (2018-12-18 02:33:56) > On Mon, 17 Dec 2018 at 19:44, Dylan Baker wrote: > > > > Quoting Emil Velikov (2018-12-17 10:58:11) > > > On Fri, 14 Dec 2018 at 17:13, Dylan Baker wrote: > > > > > > > > Quoting Emil Velikov (2018-12-13 08:05:52) > > > > > From: Emil Velikov > > > > > > > > > > The library is the dri capable one, push the check src/meson.build, > > > > > instead of the current partial handling in src/glx/meson.build. > > > > > > > > > > Fixes: a47c525f328 ("meson: build glx") > > > > > > > > This is just a refactor, the Fixes: is unnecessary. More on that below > > > > > > > If I'm reading things correctly, building with -Dglx=xlib/gallium-xlib > > > -Dshared-glapi -Denable-tests - will attempt to build the tests in > > > src/glx/tests/. > > > Which is something that shouldn't happen IMHO. > > > > Wait, why shouldn't they run? The tests pass (at least with the gallium-xlib > > glx), and running tests seems useful. > > > Fully agree running tests is useful and welcome. > > The tests flex the -Dglx=dri code in src/glx/. As-is it's rather > confusing, misleading even, to request one thing then run tests for > something that looks the same but isn't. > Silly analogy - running ANV tests, when -Dvulkan-drivers=amd is used. > > Hope this is clear - not sure if the coffee has kicked in ;-) > -Emil Sorry, I meant to respond yesterday. Ian explained why these tests are only useful if building DRI based GLX. Maybe make the commit message something like: "meson: Ensure tests for dri based glx are only built when dri based glx is enabled" or something like that? I'll leave it up to you what you think is best. As long as you note that the real functional change is not building tests when we don't want/need them. Reviewed-by: Dylan Baker signature.asc Description: signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 03/18] glx: meson: build src/glx only with -Dglx=dri
On Mon, 17 Dec 2018 at 19:44, Dylan Baker wrote: > > Quoting Emil Velikov (2018-12-17 10:58:11) > > On Fri, 14 Dec 2018 at 17:13, Dylan Baker wrote: > > > > > > Quoting Emil Velikov (2018-12-13 08:05:52) > > > > From: Emil Velikov > > > > > > > > The library is the dri capable one, push the check src/meson.build, > > > > instead of the current partial handling in src/glx/meson.build. > > > > > > > > Fixes: a47c525f328 ("meson: build glx") > > > > > > This is just a refactor, the Fixes: is unnecessary. More on that below > > > > > If I'm reading things correctly, building with -Dglx=xlib/gallium-xlib > > -Dshared-glapi -Denable-tests - will attempt to build the tests in > > src/glx/tests/. > > Which is something that shouldn't happen IMHO. > > Wait, why shouldn't they run? The tests pass (at least with the gallium-xlib > glx), and running tests seems useful. > Fully agree running tests is useful and welcome. The tests flex the -Dglx=dri code in src/glx/. As-is it's rather confusing, misleading even, to request one thing then run tests for something that looks the same but isn't. Silly analogy - running ANV tests, when -Dvulkan-drivers=amd is used. Hope this is clear - not sure if the coffee has kicked in ;-) -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 03/18] glx: meson: build src/glx only with -Dglx=dri
Quoting Emil Velikov (2018-12-17 10:58:11) > On Fri, 14 Dec 2018 at 17:13, Dylan Baker wrote: > > > > Quoting Emil Velikov (2018-12-13 08:05:52) > > > From: Emil Velikov > > > > > > The library is the dri capable one, push the check src/meson.build, > > > instead of the current partial handling in src/glx/meson.build. > > > > > > Fixes: a47c525f328 ("meson: build glx") > > > > This is just a refactor, the Fixes: is unnecessary. More on that below > > > If I'm reading things correctly, building with -Dglx=xlib/gallium-xlib > -Dshared-glapi -Denable-tests - will attempt to build the tests in > src/glx/tests/. > Which is something that shouldn't happen IMHO. Wait, why shouldn't they run? The tests pass (at least with the gallium-xlib glx), and running tests seems useful. > With this in mind, do you think we should drop the fixes tag? > > > > Signed-off-by: Emil Velikov > > > --- > > > src/glx/meson.build | 32 +++- > > > src/meson.build | 2 +- > > > 2 files changed, 16 insertions(+), 18 deletions(-) > > > > > > diff --git a/src/glx/meson.build b/src/glx/meson.build > > > index 3fd74439b11..898ed1f5826 100644 > > > --- a/src/glx/meson.build > > > +++ b/src/glx/meson.build > > > @@ -152,23 +152,21 @@ libglx = static_library( > > >build_by_default : false, > > > ) > > > > > > -if with_glx == 'dri' > > > - libgl = shared_library( > > > -gl_lib_name, > > > -[], > > > -include_directories : [inc_common, inc_glapi, inc_loader, > > > inc_gl_internal], > > > -link_with : [libglapi_static, libglapi], > > > -link_whole : libglx, > > > -link_args : [ld_args_bsymbolic, ld_args_gc_sections, > > > extra_ld_args_libgl], > > > -dependencies : [ > > > - dep_libdrm, dep_dl, dep_m, dep_thread, dep_x11, dep_xcb_glx, > > > dep_xcb, > > > - dep_x11_xcb, dep_xcb_dri2, dep_xext, dep_xfixes, dep_xdamage, > > > dep_xxf86vm, > > > - extra_deps_libgl, > > > -], > > > -version : gl_lib_version, > > > -install : true, > > > - ) > > > -endif > > > +libgl = shared_library( > > > + gl_lib_name, > > > + [], > > > + include_directories : [inc_common, inc_glapi, inc_loader, > > > inc_gl_internal], > > > + link_with : [libglapi_static, libglapi], > > > + link_whole : libglx, > > > + link_args : [ld_args_bsymbolic, ld_args_gc_sections, > > > extra_ld_args_libgl], > > > + dependencies : [ > > > +dep_libdrm, dep_dl, dep_m, dep_thread, dep_x11, dep_xcb_glx, dep_xcb, > > > +dep_x11_xcb, dep_xcb_dri2, dep_xext, dep_xfixes, dep_xdamage, > > > dep_xxf86vm, > > > +extra_deps_libgl, > > > + ], > > > + version : gl_lib_version, > > > + install : true, > > > +) > > > > All you're doing is refactoring out the build_by_default into an if > > statement. > > If you're going to do this, please remove `build_by_default : false` from > > the > > libglx in src/glx. > > > Sure thing. > > > With those changes: > > Reviewed-by: Dylan Baker > > > > Thanks. > Emil signature.asc Description: signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 03/18] glx: meson: build src/glx only with -Dglx=dri
On Fri, 14 Dec 2018 at 17:13, Dylan Baker wrote: > > Quoting Emil Velikov (2018-12-13 08:05:52) > > From: Emil Velikov > > > > The library is the dri capable one, push the check src/meson.build, > > instead of the current partial handling in src/glx/meson.build. > > > > Fixes: a47c525f328 ("meson: build glx") > > This is just a refactor, the Fixes: is unnecessary. More on that below > If I'm reading things correctly, building with -Dglx=xlib/gallium-xlib -Dshared-glapi -Denable-tests - will attempt to build the tests in src/glx/tests/. Which is something that shouldn't happen IMHO. With this in mind, do you think we should drop the fixes tag? > > Signed-off-by: Emil Velikov > > --- > > src/glx/meson.build | 32 +++- > > src/meson.build | 2 +- > > 2 files changed, 16 insertions(+), 18 deletions(-) > > > > diff --git a/src/glx/meson.build b/src/glx/meson.build > > index 3fd74439b11..898ed1f5826 100644 > > --- a/src/glx/meson.build > > +++ b/src/glx/meson.build > > @@ -152,23 +152,21 @@ libglx = static_library( > >build_by_default : false, > > ) > > > > -if with_glx == 'dri' > > - libgl = shared_library( > > -gl_lib_name, > > -[], > > -include_directories : [inc_common, inc_glapi, inc_loader, > > inc_gl_internal], > > -link_with : [libglapi_static, libglapi], > > -link_whole : libglx, > > -link_args : [ld_args_bsymbolic, ld_args_gc_sections, > > extra_ld_args_libgl], > > -dependencies : [ > > - dep_libdrm, dep_dl, dep_m, dep_thread, dep_x11, dep_xcb_glx, dep_xcb, > > - dep_x11_xcb, dep_xcb_dri2, dep_xext, dep_xfixes, dep_xdamage, > > dep_xxf86vm, > > - extra_deps_libgl, > > -], > > -version : gl_lib_version, > > -install : true, > > - ) > > -endif > > +libgl = shared_library( > > + gl_lib_name, > > + [], > > + include_directories : [inc_common, inc_glapi, inc_loader, > > inc_gl_internal], > > + link_with : [libglapi_static, libglapi], > > + link_whole : libglx, > > + link_args : [ld_args_bsymbolic, ld_args_gc_sections, > > extra_ld_args_libgl], > > + dependencies : [ > > +dep_libdrm, dep_dl, dep_m, dep_thread, dep_x11, dep_xcb_glx, dep_xcb, > > +dep_x11_xcb, dep_xcb_dri2, dep_xext, dep_xfixes, dep_xdamage, > > dep_xxf86vm, > > +extra_deps_libgl, > > + ], > > + version : gl_lib_version, > > + install : true, > > +) > > All you're doing is refactoring out the build_by_default into an if statement. > If you're going to do this, please remove `build_by_default : false` from the > libglx in src/glx. > Sure thing. > With those changes: > Reviewed-by: Dylan Baker > Thanks. Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 03/18] glx: meson: build src/glx only with -Dglx=dri
Quoting Emil Velikov (2018-12-13 08:05:52) > From: Emil Velikov > > The library is the dri capable one, push the check src/meson.build, > instead of the current partial handling in src/glx/meson.build. > > Fixes: a47c525f328 ("meson: build glx") This is just a refactor, the Fixes: is unnecessary. More on that below > Signed-off-by: Emil Velikov > --- > src/glx/meson.build | 32 +++- > src/meson.build | 2 +- > 2 files changed, 16 insertions(+), 18 deletions(-) > > diff --git a/src/glx/meson.build b/src/glx/meson.build > index 3fd74439b11..898ed1f5826 100644 > --- a/src/glx/meson.build > +++ b/src/glx/meson.build > @@ -152,23 +152,21 @@ libglx = static_library( >build_by_default : false, > ) > > -if with_glx == 'dri' > - libgl = shared_library( > -gl_lib_name, > -[], > -include_directories : [inc_common, inc_glapi, inc_loader, > inc_gl_internal], > -link_with : [libglapi_static, libglapi], > -link_whole : libglx, > -link_args : [ld_args_bsymbolic, ld_args_gc_sections, > extra_ld_args_libgl], > -dependencies : [ > - dep_libdrm, dep_dl, dep_m, dep_thread, dep_x11, dep_xcb_glx, dep_xcb, > - dep_x11_xcb, dep_xcb_dri2, dep_xext, dep_xfixes, dep_xdamage, > dep_xxf86vm, > - extra_deps_libgl, > -], > -version : gl_lib_version, > -install : true, > - ) > -endif > +libgl = shared_library( > + gl_lib_name, > + [], > + include_directories : [inc_common, inc_glapi, inc_loader, inc_gl_internal], > + link_with : [libglapi_static, libglapi], > + link_whole : libglx, > + link_args : [ld_args_bsymbolic, ld_args_gc_sections, extra_ld_args_libgl], > + dependencies : [ > +dep_libdrm, dep_dl, dep_m, dep_thread, dep_x11, dep_xcb_glx, dep_xcb, > +dep_x11_xcb, dep_xcb_dri2, dep_xext, dep_xfixes, dep_xdamage, > dep_xxf86vm, > +extra_deps_libgl, > + ], > + version : gl_lib_version, > + install : true, > +) All you're doing is refactoring out the build_by_default into an if statement. If you're going to do this, please remove `build_by_default : false` from the libglx in src/glx. With those changes: Reviewed-by: Dylan Baker > > if with_tests >subdir('tests') > diff --git a/src/meson.build b/src/meson.build > index 915441fb2ce..ae094fccf6c 100644 > --- a/src/meson.build > +++ b/src/meson.build > @@ -74,7 +74,7 @@ subdir('loader') > if with_platform_haiku >subdir('hgl') > endif > -if with_glx != 'disabled' > +if with_glx == 'dri' >subdir('glx') > endif > if with_gbm > -- > 2.19.2 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev signature.asc Description: signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 03/18] glx: meson: build src/glx only with -Dglx=dri
From: Emil Velikov The library is the dri capable one, push the check src/meson.build, instead of the current partial handling in src/glx/meson.build. Fixes: a47c525f328 ("meson: build glx") Signed-off-by: Emil Velikov --- src/glx/meson.build | 32 +++- src/meson.build | 2 +- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/src/glx/meson.build b/src/glx/meson.build index 3fd74439b11..898ed1f5826 100644 --- a/src/glx/meson.build +++ b/src/glx/meson.build @@ -152,23 +152,21 @@ libglx = static_library( build_by_default : false, ) -if with_glx == 'dri' - libgl = shared_library( -gl_lib_name, -[], -include_directories : [inc_common, inc_glapi, inc_loader, inc_gl_internal], -link_with : [libglapi_static, libglapi], -link_whole : libglx, -link_args : [ld_args_bsymbolic, ld_args_gc_sections, extra_ld_args_libgl], -dependencies : [ - dep_libdrm, dep_dl, dep_m, dep_thread, dep_x11, dep_xcb_glx, dep_xcb, - dep_x11_xcb, dep_xcb_dri2, dep_xext, dep_xfixes, dep_xdamage, dep_xxf86vm, - extra_deps_libgl, -], -version : gl_lib_version, -install : true, - ) -endif +libgl = shared_library( + gl_lib_name, + [], + include_directories : [inc_common, inc_glapi, inc_loader, inc_gl_internal], + link_with : [libglapi_static, libglapi], + link_whole : libglx, + link_args : [ld_args_bsymbolic, ld_args_gc_sections, extra_ld_args_libgl], + dependencies : [ +dep_libdrm, dep_dl, dep_m, dep_thread, dep_x11, dep_xcb_glx, dep_xcb, +dep_x11_xcb, dep_xcb_dri2, dep_xext, dep_xfixes, dep_xdamage, dep_xxf86vm, +extra_deps_libgl, + ], + version : gl_lib_version, + install : true, +) if with_tests subdir('tests') diff --git a/src/meson.build b/src/meson.build index 915441fb2ce..ae094fccf6c 100644 --- a/src/meson.build +++ b/src/meson.build @@ -74,7 +74,7 @@ subdir('loader') if with_platform_haiku subdir('hgl') endif -if with_glx != 'disabled' +if with_glx == 'dri' subdir('glx') endif if with_gbm -- 2.19.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev