On February 28, 2018 8:30:14 PM UTC, Andres Gomez <ago...@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 <dy...@pnwbakers.com> > > > > > Cc: Eric Engestrom <eric.engest...@imgtec.com> > > > > > Signed-off-by: Andres Gomez <ago...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev