[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 <agomez at igalia.com> wrote:
> On Wed, 2018-02-28 at 17:12 +0000, Eric Engestrom wrote:
> > On Wednesday, 2018-02-28 17:08:41 +0000, Eric Engestrom wrote:
> > > On Wednesday, 2018-02-28 17:02:50 +0000, 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 <dylan at pnwbakers.com>
> > > > > Cc: Eric Engestrom <eric.engestrom at imgtec.com>
> > > > > Signed-off-by: Andres Gomez <agomez at igalia.com>
> > > > > ---
> > > > >  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