[Mesa-dev] [PATCH v2 1/6] configure.ac: Move LLVM related stuff to the top

[Tobias Droste]

Hi Emil,

there's no way I can think of moving only without breaking the build for the 
following reason:
The llvm check itself depends on several global variables defined later and 
checks stuff it shouldn't do.
The moved code uses $enable_opencl and $CLANG_LIBDIR and additionally this 
code fails if LLVM is not installed, but at this point in time I don't know if 
I need LLVM!
In order to do the later patches this stuff has to be at the top.

I have to either move&change in one go or do nothing.
That's why originally this was one patch and for this series I said it should 
be squashed when pushed.

I could (but rather choose not to) split this up with duplicate code and only 
one of them enabled (so not move, but copy code) until the other works.
But this makes the already fragile configure.ac more fragile inbetween the 
steps.

I really wish I could split this up more, but I would rather not.
Could we just squash patch 1-5 after the other issues you raised are resolved 
and push it as just 2 patches?

Am Dienstag, 11. Oktober 2016, 13:30:30 CEST schrieben Sie:
> On 11 October 2016 at 02:44, Tobias Droste <tdroste at gmx.de> wrote:
> > This moves the LLVM related stuff to the top of the fileto make it
> > available to all later checks.
> > It's below the low level system checks without dependecies or heavy
> > logic.
> > 
> > WARNING: This just moves stuff, but breaks the build due to the new
> > ordering!!!
> 
> You cannot do this, sorry. Separate patches should not cause breakage
> (where possible) and patches should be split a) mechanical and b)
> functionality changes. There's more to it that just reviewing the
> work.
> 
> With a tweak below this should be fine afaict.
> 
> > Signed-off-by: Tobias Droste <tdroste at gmx.de>
> > ---
> > 
> >  configure.ac | 341
> >  ++++++++++++++++++++++++++++++----------------------------- 1 file
> >  changed, 172 insertions(+), 169 deletions(-)
> > 
> > +if test -z "${LLVM_CONFIG}"; then
> > +    if test -n "$llvm_prefix"; then
> > +        AC_PATH_TOOL([LLVM_CONFIG], [llvm-config], [no],
> > ["$llvm_prefix/bin"]) +    else
> > +        AC_PATH_TOOL([LLVM_CONFIG], [llvm-config], [no])
> > +    fi
> > +fi
> > +
> > +if test "x$LLVM_CONFIG" != xno; then
> > +    LLVM_VERSION=`$LLVM_CONFIG --version | egrep -o '^[[0-9.]]+'`
> > +    LLVM_LDFLAGS=`$LLVM_CONFIG --ldflags`
> > +    LLVM_BINDIR=`$LLVM_CONFIG --bindir`
> > +    LLVM_CPPFLAGS=`strip_unwanted_llvm_flags "$LLVM_CONFIG --cppflags"`
> > +    LLVM_CFLAGS=$LLVM_CPPFLAGS   # CPPFLAGS seem to be sufficient
> > +    LLVM_CXXFLAGS=`strip_unwanted_llvm_flags "$LLVM_CONFIG --cxxflags"`
> > +    LLVM_INCLUDEDIR=`$LLVM_CONFIG --includedir`
> > +    LLVM_LIBDIR=`$LLVM_CONFIG --libdir`
> > +
> > +    AC_COMPUTE_INT([LLVM_VERSION_MAJOR], [LLVM_VERSION_MAJOR],
> > +        [#include "${LLVM_INCLUDEDIR}/llvm/Config/llvm-config.h"])
> > +    AC_COMPUTE_INT([LLVM_VERSION_MINOR], [LLVM_VERSION_MINOR],
> > +        [#include "${LLVM_INCLUDEDIR}/llvm/Config/llvm-config.h"])
> > +
> > +    LLVM_VERSION_PATCH=`echo $LLVM_VERSION | cut -d. -f3 | egrep -o
> > '^[[0-9]]+'` +    if test -z "$LLVM_VERSION_PATCH"; then
> > +        LLVM_VERSION_PATCH=0
> > +    fi
> > +
> > +    if test -n "${LLVM_VERSION_MAJOR}"; then
> > +        LLVM_VERSION_INT="${LLVM_VERSION_MAJOR}0${LLVM_VERSION_MINOR}"
> > +    else
> > +        LLVM_VERSION_INT=`echo $LLVM_VERSION | sed -e
> > 's/\([[0-9]]\)\.\([[0-9]]\)/\10\2/g'` +    fi
> > +
> > +    LLVM_REQUIRED_VERSION_MAJOR="3"
> > +    LLVM_REQUIRED_VERSION_MINOR="3"
> > +    if test "$LLVM_VERSION_INT" -lt
> > "${LLVM_REQUIRED_VERSION_MAJOR}0${LLVM_REQUIRED_VERSION_MINOR}"; then +  
> >      AC_MSG_ERROR([LLVM
> > $LLVM_REQUIRED_VERSION_MAJOR.$LLVM_REQUIRED_VERSION_MINOR or newer is
> > required]) +    fi
> > +
> > +    LLVM_COMPONENTS="engine bitwriter mcjit mcdisassembler"
> > +
> > +    if $LLVM_CONFIG --components | grep -q inteljitevents ; then
> > +        LLVM_COMPONENTS="${LLVM_COMPONENTS} inteljitevents"
> > +    fi
> > +
> > +    if test "x$enable_opencl" = xyes; then
> > +        llvm_check_version_for "3" "6" "0" "opencl"
> > +
> > +        LLVM_COMPONENTS="${LLVM_COMPONENTS} all-targets ipo linker
> > instrumentation" +        LLVM_COMPONENTS="${LLVM_COMPONENTS} irreader
> > option objcarcopts profiledata" +    fi
> > +    DEFINES="${DEFINES} -DHAVE_LLVM=0x0$LLVM_VERSION_INT
> > -DMESA_LLVM_VERSION_PATCH=$LLVM_VERSION_PATCH" +    MESA_LLVM=1
> > +
> > +    dnl Check for Clang internal headers
> > +    if test "x$enable_opencl" = xyes; then
> > +        if test -z "$CLANG_LIBDIR"; then
> > +            CLANG_LIBDIR=${LLVM_LIBDIR}
> > +        fi
> > +        CLANG_RESOURCE_DIR=$CLANG_LIBDIR/clang/${LLVM_VERSION}
> > +        AS_IF([test ! -f "$CLANG_RESOURCE_DIR/include/stddef.h"],
> > +            [AC_MSG_ERROR([Could not find clang internal header stddef.h
> > in $CLANG_RESOURCE_DIR Use --with-clang-libdir to specify the correct
> > path to the clang libraries.])]) +    fi
> > +else
> > +    MESA_LLVM=0
> > +    LLVM_VERSION_INT=0
> > +fi
> > +
> > +AC_SUBST([MESA_LLVM])
> > +AC_SUBST([LLVM_BINDIR])
> > +AC_SUBST([LLVM_CFLAGS])
> > +AC_SUBST([LLVM_CPPFLAGS])
> > +AC_SUBST([LLVM_CXXFLAGS])
> > +AC_SUBST([LLVM_LIBDIR])
> > +AC_SUBST([LLVM_LIBS])
> > +AC_SUBST([LLVM_LDFLAGS])
> > +AC_SUBST([LLVM_INCLUDEDIR])
> > +AC_SUBST([LLVM_VERSION])
> > +
> 
> Afaics these hunks are the ones what cause both functionality change
> and breakage. As a transitional stage (or might be good all together
> ?) move the existing functionality to foo() and call it from the
> respective place.
> 
> The rest of the patch should not cause issues, although I haven't tested.
> Emil