Re: [Mesa-dev] [PATCH 1/2] meson: fix LLVM version detection when <= 3.4

[Andres Gomez]

On Thu, 2018-03-01 at 09:25 +, Eric Engestrom wrote:

[...]

> 
> Oh, my apologies, I didn't think about that!
> Can you add that paragraph in the commit message so it's clearer?
> (I know there was already a mention of that, but I had not understood it the 
> first time around)
> 
> > 
> > You can see an example of this error at:
> > https://travis-ci.org/Igalia/release-mesa/builds/347267445
> > 
> > 
> > I'll send a new version following your snippet. Thanks! ☺
> 
> You can add my r-b on that patch :)

Modified locally and landed.

Thanks a lot for the review! ☺

-- 
Br,

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

Re: [Mesa-dev] [PATCH 1/2] meson: fix LLVM version detection when <= 3.4

[Eric Engestrom]

On February 28, 2018 8:30:14 PM UTC, Andres Gomez  wrote:
> On Wed, 2018-02-28 at 17:12 +, Eric Engestrom wrote:
> > On Wednesday, 2018-02-28 17:08:41 +, Eric Engestrom wrote:
> > > On Wednesday, 2018-02-28 17:02:50 +, Eric Engestrom wrote:
> > > > On Wednesday, 2018-02-28 17:52:05 +0200, Andres Gomez wrote:
> > > > > 3 digits versions in LLVM only started from 3.4.1 on. Hence,
> if you
> > > > > have installed 3.4 or below, meson will fail even when we may
> not make
> > > > > use of LLVM.
> > > > > 
> > > > > Cc: Dylan Baker 
> > > > > Cc: Eric Engestrom 
> > > > > Signed-off-by: Andres Gomez 
> > > > > ---
> > > > >  meson.build | 13 -
> > > > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/meson.build b/meson.build
> > > > > index 308f64cf811..b8c0b04893c 100644
> > > > > --- a/meson.build
> > > > > +++ b/meson.build
> > > > > @@ -1037,7 +1037,18 @@ if with_llvm
> > > > ># that for our version checks.
> > > > ># svn suffixes are stripped by meson as of 0.43, and git
> suffixes are
> > > > ># strippped as of 0.44, but we support older meson
> versions.
> > > > > -  _llvm_patch = _llvm_version[2]
> > > > > +
> > > > > +  # 3 digits versions in LLVM only started from 3.4.1 on
> > > > > +  if dep_llvm.version() <= '3.3'
> > > > 
> > > > The correct 'meson way' to compare version strings is
> > > >   if dep_llvm.version().version_compare('<= 3.3')
> > > > 
> > > > > +_llvm_patch = _llvm_version[1]
> > > > > +  elif dep_llvm.version() >= '3.5'
> > > > > +_llvm_patch = _llvm_version[2]
> > > > > +  elif dep_llvm.version().startswith('3.4.1') or
> dep_llvm.version().startswith('3.4.2')
> > > > > +_llvm_patch = _llvm_version[2]
> > > > > +  else
> > > > > +_llvm_patch = _llvm_version[1]
> > > > > +  endif
> > > > 
> > > > This whole logic seems overly complicated, and I don't think
> duplicating
> > > > the minor version as the patch version is the right thing
> either.
> 
> Ouch! Right, minor version should be 0 in those cases.
> 
> > > > How about this instead?
> > > > 
> > > >   if dep_llvm.version().version_compare('>= 3.4.1')
> > > > _llvm_patch = _llvm_version[2]
> > > >   else
> > > > _llvm_patch = '0'
> > > > endif
> > > 
> > > Actually, do we still support llvm < 3.4? Didn't we just bump the
> > > minimum to 4.0?
> > > I think we did, in which case this patch is not necessary at all
> :)
> > 
> > Correction: the minimum is 3.9, which is still >= 3.4, so NAK on
> this
> > patch, it would be dead code anyway :)
> 
> The purpose of this patch is not to provide support for older versions
> of LLVM but avoiding meson to fail when an older version is present in
> the system.
> 
> In other words, you can perfectly build with an old LLVM (< 3.4.1) in
> the system while not needing LLVM at all (auto). When passing through
> this detection code, meson will fail when accessing "_llvm_version[2]"
> due to:
> 
> "Index 2 out of bounds of array of size 2."

Oh, my apologies, I didn't think about that!
Can you add that paragraph in the commit message so it's clearer?
(I know there was already a mention of that, but I had not understood it the 
first time around)

> 
> You can see an example of this error at:
> https://travis-ci.org/Igalia/release-mesa/builds/347267445
> 
> 
> I'll send a new version following your snippet. Thanks! ☺

You can add my r-b on that patch :)

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

Re: [Mesa-dev] [PATCH 1/2] meson: fix LLVM version detection when <= 3.4

[Andres Gomez]

On Wed, 2018-02-28 at 17:12 +, Eric Engestrom wrote:
> On Wednesday, 2018-02-28 17:08:41 +, Eric Engestrom wrote:
> > On Wednesday, 2018-02-28 17:02:50 +, Eric Engestrom wrote:
> > > On Wednesday, 2018-02-28 17:52:05 +0200, Andres Gomez wrote:
> > > > 3 digits versions in LLVM only started from 3.4.1 on. Hence, if you
> > > > have installed 3.4 or below, meson will fail even when we may not make
> > > > use of LLVM.
> > > > 
> > > > Cc: Dylan Baker 
> > > > Cc: Eric Engestrom 
> > > > Signed-off-by: Andres Gomez 
> > > > ---
> > > >  meson.build | 13 -
> > > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/meson.build b/meson.build
> > > > index 308f64cf811..b8c0b04893c 100644
> > > > --- a/meson.build
> > > > +++ b/meson.build
> > > > @@ -1037,7 +1037,18 @@ if with_llvm
> > > ># that for our version checks.
> > > ># svn suffixes are stripped by meson as of 0.43, and git suffixes are
> > > ># strippped as of 0.44, but we support older meson versions.
> > > > -  _llvm_patch = _llvm_version[2]
> > > > +
> > > > +  # 3 digits versions in LLVM only started from 3.4.1 on
> > > > +  if dep_llvm.version() <= '3.3'
> > > 
> > > The correct 'meson way' to compare version strings is
> > >   if dep_llvm.version().version_compare('<= 3.3')
> > > 
> > > > +_llvm_patch = _llvm_version[1]
> > > > +  elif dep_llvm.version() >= '3.5'
> > > > +_llvm_patch = _llvm_version[2]
> > > > +  elif dep_llvm.version().startswith('3.4.1') or 
> > > > dep_llvm.version().startswith('3.4.2')
> > > > +_llvm_patch = _llvm_version[2]
> > > > +  else
> > > > +_llvm_patch = _llvm_version[1]
> > > > +  endif
> > > 
> > > This whole logic seems overly complicated, and I don't think duplicating
> > > the minor version as the patch version is the right thing either.

Ouch! Right, minor version should be 0 in those cases.

> > > How about this instead?
> > > 
> > >   if dep_llvm.version().version_compare('>= 3.4.1')
> > > _llvm_patch = _llvm_version[2]
> > >   else
> > > _llvm_patch = '0'
> > >   endif
> > 
> > Actually, do we still support llvm < 3.4? Didn't we just bump the
> > minimum to 4.0?
> > I think we did, in which case this patch is not necessary at all :)
> 
> Correction: the minimum is 3.9, which is still >= 3.4, so NAK on this
> patch, it would be dead code anyway :)

The purpose of this patch is not to provide support for older versions
of LLVM but avoiding meson to fail when an older version is present in
the system.

In other words, you can perfectly build with an old LLVM (< 3.4.1) in
the system while not needing LLVM at all (auto). When passing through
this detection code, meson will fail when accessing "_llvm_version[2]"
due to:

"Index 2 out of bounds of array of size 2."

You can see an example of this error at:
https://travis-ci.org/Igalia/release-mesa/builds/347267445


I'll send a new version following your snippet. Thanks! ☺

-- 
Br,

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

Re: [Mesa-dev] [PATCH 1/2] meson: fix LLVM version detection when <= 3.4

[Roland Scheidegger]

Am 28.02.2018 um 18:18 schrieb Dylan Baker:
> Quoting Eric Engestrom (2018-02-28 09:08:41)
>> On Wednesday, 2018-02-28 17:02:50 +, Eric Engestrom wrote:
>>> On Wednesday, 2018-02-28 17:52:05 +0200, Andres Gomez wrote:
 3 digits versions in LLVM only started from 3.4.1 on. Hence, if you
 have installed 3.4 or below, meson will fail even when we may not make
 use of LLVM.

 Cc: Dylan Baker 
 Cc: Eric Engestrom 
 Signed-off-by: Andres Gomez 
 ---
  meson.build | 13 -
  1 file changed, 12 insertions(+), 1 deletion(-)

 diff --git a/meson.build b/meson.build
 index 308f64cf811..b8c0b04893c 100644
 --- a/meson.build
 +++ b/meson.build
 @@ -1037,7 +1037,18 @@ if with_llvm
# that for our version checks.
# svn suffixes are stripped by meson as of 0.43, and git suffixes are
# strippped as of 0.44, but we support older meson versions.
 -  _llvm_patch = _llvm_version[2]
 +
 +  # 3 digits versions in LLVM only started from 3.4.1 on
 +  if dep_llvm.version() <= '3.3'
>>>
>>> The correct 'meson way' to compare version strings is
>>>   if dep_llvm.version().version_compare('<= 3.3')
>>>
 +_llvm_patch = _llvm_version[1]
 +  elif dep_llvm.version() >= '3.5'
 +_llvm_patch = _llvm_version[2]
 +  elif dep_llvm.version().startswith('3.4.1') or 
 dep_llvm.version().startswith('3.4.2')
 +_llvm_patch = _llvm_version[2]
 +  else
 +_llvm_patch = _llvm_version[1]
 +  endif
>>>
>>> This whole logic seems overly complicated, and I don't think duplicating
>>> the minor version as the patch version is the right thing either.
>>> How about this instead?
>>>
>>>   if dep_llvm.version().version_compare('>= 3.4.1')
>>> _llvm_patch = _llvm_version[2]
>>>   else
>>> _llvm_patch = '0'
>>>   endif
>>
>> Actually, do we still support llvm < 3.4? Didn't we just bump the
>> minimum to 4.0?
>> I think we did, in which case this patch is not necessary at all :)
> 
> You can build llvmpipe with LLVM 3.3, but everything else requires a more 
> recent
> version. Maybe we can ask VMWare if they still actually care about building
> against 3.3?
> 
> Otherwise I like Eric's version better.
> 
>

We're about to switch to something newer, and since we still use scons I
suppose it's ok if you don't support such old versions with meson.

Roland


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

Re: [Mesa-dev] [PATCH 1/2] meson: fix LLVM version detection when <= 3.4

[Dylan Baker]

Quoting Eric Engestrom (2018-02-28 09:08:41)
> On Wednesday, 2018-02-28 17:02:50 +, Eric Engestrom wrote:
> > On Wednesday, 2018-02-28 17:52:05 +0200, Andres Gomez wrote:
> > > 3 digits versions in LLVM only started from 3.4.1 on. Hence, if you
> > > have installed 3.4 or below, meson will fail even when we may not make
> > > use of LLVM.
> > > 
> > > Cc: Dylan Baker 
> > > Cc: Eric Engestrom 
> > > Signed-off-by: Andres Gomez 
> > > ---
> > >  meson.build | 13 -
> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/meson.build b/meson.build
> > > index 308f64cf811..b8c0b04893c 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -1037,7 +1037,18 @@ if with_llvm
> > ># that for our version checks.
> > ># svn suffixes are stripped by meson as of 0.43, and git suffixes are
> > ># strippped as of 0.44, but we support older meson versions.
> > > -  _llvm_patch = _llvm_version[2]
> > > +
> > > +  # 3 digits versions in LLVM only started from 3.4.1 on
> > > +  if dep_llvm.version() <= '3.3'
> > 
> > The correct 'meson way' to compare version strings is
> >   if dep_llvm.version().version_compare('<= 3.3')
> > 
> > > +_llvm_patch = _llvm_version[1]
> > > +  elif dep_llvm.version() >= '3.5'
> > > +_llvm_patch = _llvm_version[2]
> > > +  elif dep_llvm.version().startswith('3.4.1') or 
> > > dep_llvm.version().startswith('3.4.2')
> > > +_llvm_patch = _llvm_version[2]
> > > +  else
> > > +_llvm_patch = _llvm_version[1]
> > > +  endif
> > 
> > This whole logic seems overly complicated, and I don't think duplicating
> > the minor version as the patch version is the right thing either.
> > How about this instead?
> > 
> >   if dep_llvm.version().version_compare('>= 3.4.1')
> > _llvm_patch = _llvm_version[2]
> >   else
> > _llvm_patch = '0'
> >   endif
> 
> Actually, do we still support llvm < 3.4? Didn't we just bump the
> minimum to 4.0?
> I think we did, in which case this patch is not necessary at all :)

You can build llvmpipe with LLVM 3.3, but everything else requires a more recent
version. Maybe we can ask VMWare if they still actually care about building
against 3.3?

Otherwise I like Eric's version better.

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 1/2] meson: fix LLVM version detection when <= 3.4

[Eric Engestrom]

On Wednesday, 2018-02-28 17:08:41 +, Eric Engestrom wrote:
> On Wednesday, 2018-02-28 17:02:50 +, Eric Engestrom wrote:
> > On Wednesday, 2018-02-28 17:52:05 +0200, Andres Gomez wrote:
> > > 3 digits versions in LLVM only started from 3.4.1 on. Hence, if you
> > > have installed 3.4 or below, meson will fail even when we may not make
> > > use of LLVM.
> > > 
> > > Cc: Dylan Baker 
> > > Cc: Eric Engestrom 
> > > Signed-off-by: Andres Gomez 
> > > ---
> > >  meson.build | 13 -
> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/meson.build b/meson.build
> > > index 308f64cf811..b8c0b04893c 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -1037,7 +1037,18 @@ if with_llvm
> > ># that for our version checks.
> > ># svn suffixes are stripped by meson as of 0.43, and git suffixes are
> > ># strippped as of 0.44, but we support older meson versions.
> > > -  _llvm_patch = _llvm_version[2]
> > > +
> > > +  # 3 digits versions in LLVM only started from 3.4.1 on
> > > +  if dep_llvm.version() <= '3.3'
> > 
> > The correct 'meson way' to compare version strings is
> >   if dep_llvm.version().version_compare('<= 3.3')
> > 
> > > +_llvm_patch = _llvm_version[1]
> > > +  elif dep_llvm.version() >= '3.5'
> > > +_llvm_patch = _llvm_version[2]
> > > +  elif dep_llvm.version().startswith('3.4.1') or 
> > > dep_llvm.version().startswith('3.4.2')
> > > +_llvm_patch = _llvm_version[2]
> > > +  else
> > > +_llvm_patch = _llvm_version[1]
> > > +  endif
> > 
> > This whole logic seems overly complicated, and I don't think duplicating
> > the minor version as the patch version is the right thing either.
> > How about this instead?
> > 
> >   if dep_llvm.version().version_compare('>= 3.4.1')
> > _llvm_patch = _llvm_version[2]
> >   else
> > _llvm_patch = '0'
> > endif
> 
> Actually, do we still support llvm < 3.4? Didn't we just bump the
> minimum to 4.0?
> I think we did, in which case this patch is not necessary at all :)

Correction: the minimum is 3.9, which is still >= 3.4, so NAK on this
patch, it would be dead code anyway :)

> 
> > 
> > > +
> > >if _llvm_patch.endswith('svn')
> > >  _llvm_patch = _llvm_patch.split('s')[0]
> > >elif _llvm_patch.contains('git')
> > > -- 
> > > 2.15.1
> > > 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] meson: fix LLVM version detection when <= 3.4

[Eric Engestrom]

On Wednesday, 2018-02-28 17:02:50 +, Eric Engestrom wrote:
> On Wednesday, 2018-02-28 17:52:05 +0200, Andres Gomez wrote:
> > 3 digits versions in LLVM only started from 3.4.1 on. Hence, if you
> > have installed 3.4 or below, meson will fail even when we may not make
> > use of LLVM.
> > 
> > Cc: Dylan Baker 
> > Cc: Eric Engestrom 
> > Signed-off-by: Andres Gomez 
> > ---
> >  meson.build | 13 -
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/meson.build b/meson.build
> > index 308f64cf811..b8c0b04893c 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -1037,7 +1037,18 @@ if with_llvm
> ># that for our version checks.
> ># svn suffixes are stripped by meson as of 0.43, and git suffixes are
> ># strippped as of 0.44, but we support older meson versions.
> > -  _llvm_patch = _llvm_version[2]
> > +
> > +  # 3 digits versions in LLVM only started from 3.4.1 on
> > +  if dep_llvm.version() <= '3.3'
> 
> The correct 'meson way' to compare version strings is
>   if dep_llvm.version().version_compare('<= 3.3')
> 
> > +_llvm_patch = _llvm_version[1]
> > +  elif dep_llvm.version() >= '3.5'
> > +_llvm_patch = _llvm_version[2]
> > +  elif dep_llvm.version().startswith('3.4.1') or 
> > dep_llvm.version().startswith('3.4.2')
> > +_llvm_patch = _llvm_version[2]
> > +  else
> > +_llvm_patch = _llvm_version[1]
> > +  endif
> 
> This whole logic seems overly complicated, and I don't think duplicating
> the minor version as the patch version is the right thing either.
> How about this instead?
> 
>   if dep_llvm.version().version_compare('>= 3.4.1')
> _llvm_patch = _llvm_version[2]
>   else
> _llvm_patch = '0'
>   endif

Actually, do we still support llvm < 3.4? Didn't we just bump the
minimum to 4.0?
I think we did, in which case this patch is not necessary at all :)

> 
> > +
> >if _llvm_patch.endswith('svn')
> >  _llvm_patch = _llvm_patch.split('s')[0]
> >elif _llvm_patch.contains('git')
> > -- 
> > 2.15.1
> > 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] meson: fix LLVM version detection when <= 3.4

[Eric Engestrom]

On Wednesday, 2018-02-28 17:52:05 +0200, Andres Gomez wrote:
> 3 digits versions in LLVM only started from 3.4.1 on. Hence, if you
> have installed 3.4 or below, meson will fail even when we may not make
> use of LLVM.
> 
> Cc: Dylan Baker 
> Cc: Eric Engestrom 
> Signed-off-by: Andres Gomez 
> ---
>  meson.build | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/meson.build b/meson.build
> index 308f64cf811..b8c0b04893c 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1037,7 +1037,18 @@ if with_llvm
># that for our version checks.
># svn suffixes are stripped by meson as of 0.43, and git suffixes are
># strippped as of 0.44, but we support older meson versions.
> -  _llvm_patch = _llvm_version[2]
> +
> +  # 3 digits versions in LLVM only started from 3.4.1 on
> +  if dep_llvm.version() <= '3.3'

The correct 'meson way' to compare version strings is
  if dep_llvm.version().version_compare('<= 3.3')

> +_llvm_patch = _llvm_version[1]
> +  elif dep_llvm.version() >= '3.5'
> +_llvm_patch = _llvm_version[2]
> +  elif dep_llvm.version().startswith('3.4.1') or 
> dep_llvm.version().startswith('3.4.2')
> +_llvm_patch = _llvm_version[2]
> +  else
> +_llvm_patch = _llvm_version[1]
> +  endif

This whole logic seems overly complicated, and I don't think duplicating
the minor version as the patch version is the right thing either.
How about this instead?

  if dep_llvm.version().version_compare('>= 3.4.1')
_llvm_patch = _llvm_version[2]
  else
_llvm_patch = '0'
endif

> +
>if _llvm_patch.endswith('svn')
>  _llvm_patch = _llvm_patch.split('s')[0]
>elif _llvm_patch.contains('git')
> -- 
> 2.15.1
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/2] meson: fix LLVM version detection when <= 3.4

[Andres Gomez]

3 digits versions in LLVM only started from 3.4.1 on. Hence, if you
have installed 3.4 or below, meson will fail even when we may not make
use of LLVM.

Cc: Dylan Baker 
Cc: Eric Engestrom 
Signed-off-by: Andres Gomez 
---
 meson.build | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 308f64cf811..b8c0b04893c 100644
--- a/meson.build
+++ b/meson.build
@@ -1037,7 +1037,18 @@ if with_llvm
   # that for our version checks.
   # svn suffixes are stripped by meson as of 0.43, and git suffixes are
   # strippped as of 0.44, but we support older meson versions.
-  _llvm_patch = _llvm_version[2]
+
+  # 3 digits versions in LLVM only started from 3.4.1 on
+  if dep_llvm.version() <= '3.3'
+_llvm_patch = _llvm_version[1]
+  elif dep_llvm.version() >= '3.5'
+_llvm_patch = _llvm_version[2]
+  elif dep_llvm.version().startswith('3.4.1') or 
dep_llvm.version().startswith('3.4.2')
+_llvm_patch = _llvm_version[2]
+  else
+_llvm_patch = _llvm_version[1]
+  endif
+
   if _llvm_patch.endswith('svn')
 _llvm_patch = _llvm_patch.split('s')[0]
   elif _llvm_patch.contains('git')
-- 
2.15.1

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