Re: [Mesa-dev] [PATCH] meson: fix some more defines meson.build

[Eric Engestrom]

On Thursday, 2018-01-25 10:31:12 +0100, Marc Dietrich wrote:
> Am Donnerstag, 25. Januar 2018, 10:28:26 CET schrieb Marc Dietrich:
> > Am Donnerstag, 25. Januar 2018, 10:18:16 CET schrieb Eric Engestrom:
> > > On Wednesday, 2018-01-24 22:02:42 +0100, Marc Dietrich wrote:
> > > > Btw, there is still some strange problem in PACKAGE_BUGREPORT as it
> > > > includes a "$" in the url. I don't know where this comes from.
> > > 
> > > Where do you see this "$"?
> > > I've looked at the code and it looks all good to me.
> > 
> > yes, code is fine, output is not (see build.ninja):
> > 
> > '-DPACKAGE_BUGREPORT="https$://bugs.freedesktop.org/enter_bug.cgi?
> > product=Mesa"'
> > 
> > maybe some escaping required?
> > 
> > Marc
> 
> arr, I just checked the resulting binary and it seems to be ok there, so 
> false 
> alarm. Still puzzling where it came from and where it went to.

This is some ninja-specific escaping:
https://ninja-build.org/manual.html#_lexical_syntax

> 
> Marc
> 


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] meson: fix some more defines meson.build

[Eric Engestrom]

On Wednesday, 2018-01-24 22:02:42 +0100, Marc Dietrich wrote:
> Btw, there is still some strange problem in PACKAGE_BUGREPORT as it
> includes a "$" in the url. I don't know where this comes from.

Where do you see this "$"?
I've looked at the code and it looks all good to me.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] meson: fix some more defines meson.build

[Dylan Baker]

Quoting Marc Dietrich (2018-01-24 13:02:42)
> Hi,
> 
> Am Mittwoch, 24. Januar 2018, 18:41:17 CET schrieb Eric Engestrom:
> > On Wednesday, 2018-01-24 17:05:52 +0100, Marc Dietrich wrote:
> > > Second hunk of fixes found by manual comparison with autotools generated
> > > compiler flags.
> > > 
> > > Signed-off-by: Marc Dietrich 
> > > ---
> > > 
> > >  - Why do we need two version macros?
> > >  - And why do we either define DEBUG or NDEBUG?
> > >  - Also autotools define some PACKAGE_ macros which are never used - maybe
> > >  
> > >time for a cleanup...
> > >  
> > >  meson.build | 8 +---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/meson.build b/meson.build
> > > index 9e3b98641f..62205aa250 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -37,7 +37,7 @@ pre_args = [
> > > 
> > >'-D__STDC_FORMAT_MACROS',
> > >'-D__STDC_LIMIT_MACROS',
> > >'-DVERSION="@0@"'.format(meson.project_version()),
> > > 
> > > -  '-DPACKAGE_VERSION=VERSION',
> > > +  '-DPACKAGE_VERSION="@0@"'.format(meson.project_version()),
> > 
> > Are you sure this is needed?
> > I haven't tested it, but I would think this tells cpp that PACKAGE_VERSION
> > has whatever value VERSION has, which is what we want.
> 
> sorry, I didn't looked too deep into it but it seems your are right.
> 
> > >'-DPACKAGE_BUGREPORT="https://bugs.freedesktop.org/enter_bug.cgi?produc
> > >t=Mesa"',>  
> > >  ]
> > > 
> > > @@ -648,6 +648,8 @@ endif
> > > 
> > >  # Define DEBUG for debug builds only (debugoptimized is not included on
> > >  this one) if get_option('buildtype') == 'debug'
> > >  
> > >pre_args += '-DDEBUG'
> > > 
> > > +else
> > > +  pre_args += '-DNDEBUG'
> > 
> > NAK on this hunk.
> > 
> > In meson, asserts and other things depending on NDEBUG are controlled by
> > `-D b_ndebug=true|false`, and shouldn't be overridden inside meson.build
> 
> ok, this is new to me and somehow not obvious.

Yeah, no other build system I'm aware of does this, but it is documented in
docs/meson.html.

> 
> > The two hunks below are correct though, thank you for finding these :)
> > Could you re-send the patch with these tags?
> > 
> > Fixes: 84486f64626ad2b51291b "meson: Enable SSE4.1 optimizations"
> > Fixes: 673dda8330769309a319d "meson: build "radv" vulkan driver for radeon
> > hardware" Cc: Dylan Baker 
> > Reviewed-by: Eric Engestrom 
> > 
> > (If you're feeling extra-motivated, you can even split these into two
> > separate patches :P)
> > 
> > >  endif
> > >  
> > >  if get_option('shader-cache')
> > > 
> > > @@ -762,7 +764,7 @@ foreach a : ['-Werror=pointer-arith', '-Werror=vla']
> > > 
> > >  endforeach
> > >  
> > >  if host_machine.cpu_family().startswith('x86')
> > > 
> > > -  pre_args += '-DHAVE_SSE41'
> > > +  pre_args += '-DUSE_SSE41'
> > > 
> > >with_sse41 = true
> > >sse41_args = ['-msse4.1']
> > > 
> > > @@ -1015,7 +1017,7 @@ if with_llvm
> > > 
> > >  _llvm_patch = _llvm_patch.split('s')[0]
> > >
> > >endif
> > >pre_args += [
> > > 
> > > -'-DHAVE_LLVM=0x0@0@@1@@2@'.format(_llvm_version[0], _llvm_version[1],
> > > _llvm_patch), +'-DHAVE_LLVM=0x0@0@0@1@'.format(_llvm_version[0],
> > > _llvm_version[1]),> 
> > >  '-DMESA_LLVM_VERSION_PATCH=@0@'.format(_llvm_patch),
> > >
> > >]
> > >  
> > >  elif with_amd_vk or with_gallium_radeonsi or with_gallium_swr
> 
> I will resend the two hunks. Btw, there is still some strange problem in 
> PACKAGE_BUGREPORT as it includes a "$" in the url. I don't know where this 
> comes from.
> 
> Marc


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] meson: fix some more defines meson.build

[Marc Dietrich]

Hi,

Am Mittwoch, 24. Januar 2018, 18:41:17 CET schrieb Eric Engestrom:
> On Wednesday, 2018-01-24 17:05:52 +0100, Marc Dietrich wrote:
> > Second hunk of fixes found by manual comparison with autotools generated
> > compiler flags.
> > 
> > Signed-off-by: Marc Dietrich 
> > ---
> > 
> >  - Why do we need two version macros?
> >  - And why do we either define DEBUG or NDEBUG?
> >  - Also autotools define some PACKAGE_ macros which are never used - maybe
> >  
> >time for a cleanup...
> >  
> >  meson.build | 8 +---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/meson.build b/meson.build
> > index 9e3b98641f..62205aa250 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -37,7 +37,7 @@ pre_args = [
> > 
> >'-D__STDC_FORMAT_MACROS',
> >'-D__STDC_LIMIT_MACROS',
> >'-DVERSION="@0@"'.format(meson.project_version()),
> > 
> > -  '-DPACKAGE_VERSION=VERSION',
> > +  '-DPACKAGE_VERSION="@0@"'.format(meson.project_version()),
> 
> Are you sure this is needed?
> I haven't tested it, but I would think this tells cpp that PACKAGE_VERSION
> has whatever value VERSION has, which is what we want.

sorry, I didn't looked too deep into it but it seems your are right.

> >'-DPACKAGE_BUGREPORT="https://bugs.freedesktop.org/enter_bug.cgi?produc
> >t=Mesa"',>  
> >  ]
> > 
> > @@ -648,6 +648,8 @@ endif
> > 
> >  # Define DEBUG for debug builds only (debugoptimized is not included on
> >  this one) if get_option('buildtype') == 'debug'
> >  
> >pre_args += '-DDEBUG'
> > 
> > +else
> > +  pre_args += '-DNDEBUG'
> 
> NAK on this hunk.
> 
> In meson, asserts and other things depending on NDEBUG are controlled by
> `-D b_ndebug=true|false`, and shouldn't be overridden inside meson.build

ok, this is new to me and somehow not obvious.

> The two hunks below are correct though, thank you for finding these :)
> Could you re-send the patch with these tags?
> 
> Fixes: 84486f64626ad2b51291b "meson: Enable SSE4.1 optimizations"
> Fixes: 673dda8330769309a319d "meson: build "radv" vulkan driver for radeon
> hardware" Cc: Dylan Baker 
> Reviewed-by: Eric Engestrom 
> 
> (If you're feeling extra-motivated, you can even split these into two
> separate patches :P)
> 
> >  endif
> >  
> >  if get_option('shader-cache')
> > 
> > @@ -762,7 +764,7 @@ foreach a : ['-Werror=pointer-arith', '-Werror=vla']
> > 
> >  endforeach
> >  
> >  if host_machine.cpu_family().startswith('x86')
> > 
> > -  pre_args += '-DHAVE_SSE41'
> > +  pre_args += '-DUSE_SSE41'
> > 
> >with_sse41 = true
> >sse41_args = ['-msse4.1']
> > 
> > @@ -1015,7 +1017,7 @@ if with_llvm
> > 
> >  _llvm_patch = _llvm_patch.split('s')[0]
> >
> >endif
> >pre_args += [
> > 
> > -'-DHAVE_LLVM=0x0@0@@1@@2@'.format(_llvm_version[0], _llvm_version[1],
> > _llvm_patch), +'-DHAVE_LLVM=0x0@0@0@1@'.format(_llvm_version[0],
> > _llvm_version[1]),> 
> >  '-DMESA_LLVM_VERSION_PATCH=@0@'.format(_llvm_patch),
> >
> >]
> >  
> >  elif with_amd_vk or with_gallium_radeonsi or with_gallium_swr

I will resend the two hunks. Btw, there is still some strange problem in 
PACKAGE_BUGREPORT as it includes a "$" in the url. I don't know where this 
comes from.

Marc


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] meson: fix some more defines meson.build

[Dylan Baker]

Quoting Eric Engestrom (2018-01-24 09:41:17)
> On Wednesday, 2018-01-24 17:05:52 +0100, Marc Dietrich wrote:
> > Second hunk of fixes found by manual comparison with autotools generated
> > compiler flags.
> > 
> > Signed-off-by: Marc Dietrich 
> > ---
> >  - Why do we need two version macros?
> >  - And why do we either define DEBUG or NDEBUG?
> >  - Also autotools define some PACKAGE_ macros which are never used - maybe
> >time for a cleanup...
> > 
> >  meson.build | 8 +---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/meson.build b/meson.build
> > index 9e3b98641f..62205aa250 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -37,7 +37,7 @@ pre_args = [
> >'-D__STDC_FORMAT_MACROS',
> >'-D__STDC_LIMIT_MACROS',
> >'-DVERSION="@0@"'.format(meson.project_version()),
> > -  '-DPACKAGE_VERSION=VERSION',
> > +  '-DPACKAGE_VERSION="@0@"'.format(meson.project_version()),
> 
> Are you sure this is needed?
> I haven't tested it, but I would think this tells cpp that PACKAGE_VERSION
> has whatever value VERSION has, which is what we want.
> 
> >
> > '-DPACKAGE_BUGREPORT="https://bugs.freedesktop.org/enter_bug.cgi?product=Mesa;',
> >  ]
> >  
> > @@ -648,6 +648,8 @@ endif
> >  # Define DEBUG for debug builds only (debugoptimized is not included on 
> > this one)
> >  if get_option('buildtype') == 'debug'
> >pre_args += '-DDEBUG'
> > +else
> > +  pre_args += '-DNDEBUG'
> 
> NAK on this hunk.
> 
> In meson, asserts and other things depending on NDEBUG are controlled by
> `-D b_ndebug=true|false`, and shouldn't be overridden inside meson.build
> 
> 
> The two hunks below are correct though, thank you for finding these :)
> Could you re-send the patch with these tags?
> 
> Fixes: 84486f64626ad2b51291b "meson: Enable SSE4.1 optimizations"
> Fixes: 673dda8330769309a319d "meson: build "radv" vulkan driver for radeon 
> hardware"
> Cc: Dylan Baker 
> Reviewed-by: Eric Engestrom 
> 
> (If you're feeling extra-motivated, you can even split these into two
> separate patches :P)
> 

Eric beat me to it :)

I'd be okay with a single patch as well, but splitting them would be preferred.

Dylan


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] meson: fix some more defines meson.build

[Dylan Baker]

Quoting Marc Dietrich (2018-01-24 08:05:52)
> Second hunk of fixes found by manual comparison with autotools generated
> compiler flags.
> 
> Signed-off-by: Marc Dietrich 
> ---
>  - Why do we need two version macros?
>  - And why do we either define DEBUG or NDEBUG?
>  - Also autotools define some PACKAGE_ macros which are never used - maybe
>time for a cleanup...
> 
>  meson.build | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index 9e3b98641f..62205aa250 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -37,7 +37,7 @@ pre_args = [
>'-D__STDC_FORMAT_MACROS',
>'-D__STDC_LIMIT_MACROS',
>'-DVERSION="@0@"'.format(meson.project_version()),
> -  '-DPACKAGE_VERSION=VERSION',
> +  '-DPACKAGE_VERSION="@0@"'.format(meson.project_version()),
>
> '-DPACKAGE_BUGREPORT="https://bugs.freedesktop.org/enter_bug.cgi?product=Mesa;',
>  ]

This hunk is just stylistic, after the pre-processor runs they will be
identical, that said I don't have a problem merging this.

>  
> @@ -648,6 +648,8 @@ endif
>  # Define DEBUG for debug builds only (debugoptimized is not included on this 
> one)
>  if get_option('buildtype') == 'debug'
>pre_args += '-DDEBUG'
> +else
> +  pre_args += '-DNDEBUG'
>  endif

NAK on this hunk. meson has -Db_ndebug for controlling NDEBUG.

DEBUG is a mesa thing for turning expensive debug code on and off, NDEBUG is for
controlling asserts; for example, the NIR validator (which is pretty expensive)
is controlled by DEBUG.

>  
>  if get_option('shader-cache')
> @@ -762,7 +764,7 @@ foreach a : ['-Werror=pointer-arith', '-Werror=vla']
>  endforeach
>  
>  if host_machine.cpu_family().startswith('x86')
> -  pre_args += '-DHAVE_SSE41'
> +  pre_args += '-DUSE_SSE41'
>with_sse41 = true
>sse41_args = ['-msse4.1']

Fixes: 84486f64626a ("meson: Enable SSE4.1 optimizations")

>  
> @@ -1015,7 +1017,7 @@ if with_llvm
>  _llvm_patch = _llvm_patch.split('s')[0]
>endif
>pre_args += [
> -'-DHAVE_LLVM=0x0@0@@1@@2@'.format(_llvm_version[0], _llvm_version[1], 
> _llvm_patch),
> +'-DHAVE_LLVM=0x0@0@0@1@'.format(_llvm_version[0], _llvm_version[1]),
>  '-DMESA_LLVM_VERSION_PATCH=@0@'.format(_llvm_patch),

Fixes: e6418ab1566d ("meson: build "radv" vulkan driver for radeon hardware")

>]
>  elif with_amd_vk or with_gallium_radeonsi or with_gallium_swr
> -- 
> 2.16.0
> 

The rest of this looks good, but it needs to be split into several patches. Each
one of them fixes a bug (except the 1st which is just a stylistic change). I've
commented inline with the Fixes tag each patch needs in it's commit message so
they get pulled into the 18.0 release.

Dylan


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] meson: fix some more defines meson.build

[Eric Engestrom]

On Wednesday, 2018-01-24 17:05:52 +0100, Marc Dietrich wrote:
> Second hunk of fixes found by manual comparison with autotools generated
> compiler flags.
> 
> Signed-off-by: Marc Dietrich 
> ---
>  - Why do we need two version macros?
>  - And why do we either define DEBUG or NDEBUG?
>  - Also autotools define some PACKAGE_ macros which are never used - maybe
>time for a cleanup...
> 
>  meson.build | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index 9e3b98641f..62205aa250 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -37,7 +37,7 @@ pre_args = [
>'-D__STDC_FORMAT_MACROS',
>'-D__STDC_LIMIT_MACROS',
>'-DVERSION="@0@"'.format(meson.project_version()),
> -  '-DPACKAGE_VERSION=VERSION',
> +  '-DPACKAGE_VERSION="@0@"'.format(meson.project_version()),

Are you sure this is needed?
I haven't tested it, but I would think this tells cpp that PACKAGE_VERSION
has whatever value VERSION has, which is what we want.

>
> '-DPACKAGE_BUGREPORT="https://bugs.freedesktop.org/enter_bug.cgi?product=Mesa;',
>  ]
>  
> @@ -648,6 +648,8 @@ endif
>  # Define DEBUG for debug builds only (debugoptimized is not included on this 
> one)
>  if get_option('buildtype') == 'debug'
>pre_args += '-DDEBUG'
> +else
> +  pre_args += '-DNDEBUG'

NAK on this hunk.

In meson, asserts and other things depending on NDEBUG are controlled by
`-D b_ndebug=true|false`, and shouldn't be overridden inside meson.build


The two hunks below are correct though, thank you for finding these :)
Could you re-send the patch with these tags?

Fixes: 84486f64626ad2b51291b "meson: Enable SSE4.1 optimizations"
Fixes: 673dda8330769309a319d "meson: build "radv" vulkan driver for radeon 
hardware"
Cc: Dylan Baker 
Reviewed-by: Eric Engestrom 

(If you're feeling extra-motivated, you can even split these into two
separate patches :P)

>  endif
>  
>  if get_option('shader-cache')
> @@ -762,7 +764,7 @@ foreach a : ['-Werror=pointer-arith', '-Werror=vla']
>  endforeach
>  
>  if host_machine.cpu_family().startswith('x86')
> -  pre_args += '-DHAVE_SSE41'
> +  pre_args += '-DUSE_SSE41'
>with_sse41 = true
>sse41_args = ['-msse4.1']
>  
> @@ -1015,7 +1017,7 @@ if with_llvm
>  _llvm_patch = _llvm_patch.split('s')[0]
>endif
>pre_args += [
> -'-DHAVE_LLVM=0x0@0@@1@@2@'.format(_llvm_version[0], _llvm_version[1], 
> _llvm_patch),
> +'-DHAVE_LLVM=0x0@0@0@1@'.format(_llvm_version[0], _llvm_version[1]),
>  '-DMESA_LLVM_VERSION_PATCH=@0@'.format(_llvm_patch),
>]
>  elif with_amd_vk or with_gallium_radeonsi or with_gallium_swr
> -- 
> 2.16.0
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] meson: fix some more defines meson.build

[Marc Dietrich]

Second hunk of fixes found by manual comparison with autotools generated
compiler flags.

Signed-off-by: Marc Dietrich 
---
 - Why do we need two version macros?
 - And why do we either define DEBUG or NDEBUG?
 - Also autotools define some PACKAGE_ macros which are never used - maybe
   time for a cleanup...

 meson.build | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/meson.build b/meson.build
index 9e3b98641f..62205aa250 100644
--- a/meson.build
+++ b/meson.build
@@ -37,7 +37,7 @@ pre_args = [
   '-D__STDC_FORMAT_MACROS',
   '-D__STDC_LIMIT_MACROS',
   '-DVERSION="@0@"'.format(meson.project_version()),
-  '-DPACKAGE_VERSION=VERSION',
+  '-DPACKAGE_VERSION="@0@"'.format(meson.project_version()),
   
'-DPACKAGE_BUGREPORT="https://bugs.freedesktop.org/enter_bug.cgi?product=Mesa;',
 ]
 
@@ -648,6 +648,8 @@ endif
 # Define DEBUG for debug builds only (debugoptimized is not included on this 
one)
 if get_option('buildtype') == 'debug'
   pre_args += '-DDEBUG'
+else
+  pre_args += '-DNDEBUG'
 endif
 
 if get_option('shader-cache')
@@ -762,7 +764,7 @@ foreach a : ['-Werror=pointer-arith', '-Werror=vla']
 endforeach
 
 if host_machine.cpu_family().startswith('x86')
-  pre_args += '-DHAVE_SSE41'
+  pre_args += '-DUSE_SSE41'
   with_sse41 = true
   sse41_args = ['-msse4.1']
 
@@ -1015,7 +1017,7 @@ if with_llvm
 _llvm_patch = _llvm_patch.split('s')[0]
   endif
   pre_args += [
-'-DHAVE_LLVM=0x0@0@@1@@2@'.format(_llvm_version[0], _llvm_version[1], 
_llvm_patch),
+'-DHAVE_LLVM=0x0@0@0@1@'.format(_llvm_version[0], _llvm_version[1]),
 '-DMESA_LLVM_VERSION_PATCH=@0@'.format(_llvm_patch),
   ]
 elif with_amd_vk or with_gallium_radeonsi or with_gallium_swr
-- 
2.16.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev