[Mesa-dev] [PATCH] configure.ac: Rework LLVM dependency handling

[Tobias Droste]

Hi Emil,

comments below.

Am Montag, 10. Oktober 2016, 13:36:15 CEST schrieben Sie:
> Hi Tobias,
> 
> On 8 October 2016 at 16:55, Tobias Droste <tdroste at gmx.de> wrote:
> > After this change each driver can request LLVM with a specific version
> > and specific targets/components anywhere.
> > 
> > For gallium "--enable-gallium-llvm" is only needed if at least one
> > driver calls "gallium_require_llvm()".
> > If the flag is set to auto it will default to no now if no driver with
> > "gallium_require_llvm()" is included.
> > 
> > If gallium swrast driver is selected and LLVM is installed and
> > "--enable-gallium-llvm" is yes or auto, it defaults to llvmpipe,
> > otherwise to softpipe.
> > 
> > Signed-off-by: Tobias Droste <tdroste at gmx.de>
> > ---
> > 
> >  configure.ac | 451
> >  +++++++++++++++++++++++++++++++++-------------------------- 1 file
> >  changed, 249 insertions(+), 202 deletions(-)
> 
> Huge thanks for starting to unwravel this.
> 
> Since the configure code is quite picky, please split out the code
> movement from the functionality changes.

This is not easy as moving the code breaks it. If it's just for review and 
doesn't need to work then I could do that.

> 
> > +llvm_add_component() {
> > +    new_llvm_component=$1
> > +    fail_if_not_present=$2
> > +    driver_name=$3
> > +
> > +    if test -z "$LLVM_COMPONENTS"; then
> > +        # Make the string non zero length
> > +        LLVM_COMPONENTS=" "
> > +
> 
> I'm not sure that this will work. For example
> $unset FOO && export FOO=" " && test -z $FOO && echo A
> A

It works that way. 
But you're right, it's probably better your way (below).

> 
> > +        # Required default components
> > +        llvm_add_component "engine" "yes" $driver_name
> > +        llvm_add_component "bitwriter" "yes" $driver_name
> > +        llvm_add_component "mcjit" "yes" $driver_name
> > +        llvm_add_component "mcdisassembler" "yes" $driver_name
> > +
> 
> At the same time I'm not sure that calling a function recursively will
> end up well, in the long run.
> 
> That said using add_compulsory (mandatory/other) and add_optional
> helpers would be better than the current "yes" "no", wouldn't it ?

I'm going with "add_*" and "add_*_optional" and remove the recursion.

> 
> Thanks
> Emil