[Mesa-dev] [PATCH v3 02/25] configure.ac: Add helper function for targets/components

[Emil Velikov]

On 12 October 2016 at 19:44, Tobias Droste <tdroste at gmx.de> wrote:
> Am Mittwoch, 12. Oktober 2016, 19:38:56 CEST schrieb Emil Velikov:
>> On 12 October 2016 at 19:01, Tobias Droste <tdroste at gmx.de> wrote:
>> > Am Mittwoch, 12. Oktober 2016, 09:52:51 CEST schrieb Emil Velikov:
>> >> On 12 October 2016 at 00:02, Tobias Droste <tdroste at gmx.de> wrote:
>> >> > Add functions to add and check targets/components.
>> >> > Not used in this patch.
>> >> >
>> >> > The error message in llvm_add_component is disabled until it doesn't
>> >> > break
>> >> > the build anymore. This is the same functionality as before where the
>> >> > components were added without a check.
>> >> >
>> >> > Signed-off-by: Tobias Droste <tdroste at gmx.de>
>> >> > ---
>> >> >
>> >> >  configure.ac | 35 +++++++++++++++++++++++++++++++++++
>> >> >  1 file changed, 35 insertions(+)
>> >> >
>> >> > diff --git a/configure.ac b/configure.ac
>> >> > index bdd46bc..69421ff 100644
>> >> > --- a/configure.ac
>> >> > +++ b/configure.ac
>> >> > @@ -2196,7 +2196,42 @@ llvm_check_version_for() {
>> >> >
>> >> >      fi
>> >> >
>> >> >  }
>> >> >
>> >> > +llvm_add_default_components() {
>> >> > +    driver_name=$1
>> >> >
>> >> > +    # Required default components
>> >> > +    llvm_add_component "engine" $driver_name
>> >> > +    llvm_add_component "bitwriter" $driver_name
>> >> > +    llvm_add_component "mcjit" $driver_name
>> >> > +    llvm_add_component "mcdisassembler" $driver_name
>> >> > +
>> >> > +    # Optional default components
>> >> > +    if $LLVM_CONFIG --components | grep -iqw inteljitevents ; then
>> >> > +        LLVM_COMPONENTS="${LLVM_COMPONENTS} inteljitevents"
>> >> > +    fi
>> >> > +}
>> >> > +
>> >> > +llvm_add_component() {
>> >>
>> >> Nit: move this function before its user (llvm_add_default_components)
>> >> Idea: call this ...components (plural) and feed the all components at
>> >> once.
>> >
>> > Ok.
>>
>> Just realised that you're adding these only to move them further up
>> with later patch. What's the obstacle for doing so in the first place
>> ?
>
> Nothing, but I was already on path 20 when I realised it and was to lazy to
> switch the ordering of the patches ;-)
>
>>
>> >> > +    new_llvm_component=$1
>> >> > +    driver_name=$2
>> >> > +
>> >> > +    if $LLVM_CONFIG --components | grep -iqw $new_llvm_component ;
>> >> > then
>> >> > +        LLVM_COMPONENTS="${LLVM_COMPONENTS} ${new_llvm_component}"
>> >> > +    #else
>> >> > +    #    AC_MSG_ERROR([LLVM component '$new_llvm_component' not
>> >> > enabled
>> >> > in your LLVM build. Required by $driver_name.])
>> >>
>> >> This function adds required components, correct ? Then the above two
>> >> lines should not be commented out.
>> >
>> > I can't enable this until later in the series as mentioned in the commit
>> > message.
>> > This is how it worked before (there were no errors for the components).
>> >
>> > At this point llvm_add_default_components will be added even if we don't
>> > need LLVM. This way someone without LLVM installed will get error
>> > messages even though he might just want to build the intel or softpipe
>> > driver.
>> > It will be enabled in Patch 24.
>>
>> If things error out here, that means that one should finish the other
>> refactoring before using these helpers ?
>> If that's getting too hairy, just add keep the function a simple
>> wrapper around LLVM_COMPONENTS (no LLVM_CONFIG or AC_MSG_ERROR) and
>> add those when it's safe.
>
> Right now this is just a wrapper around LLVM_COMPONENTS.
Atm, is more than just a dummy wrapper around LLVM_COMPONENTS.

> If a component does not exist it will later not generate any LLVM_LIBS that's
> why it also doesn't need to be in LLVM_COMPONENTS.
> If it actually is missing, you will only notice during linking / building
> (depending on wether headers are missing or not).
>
Looking at the current code - you only _need_  the --components | grep
foo check for the inteljitevents case.
Where/how do you see an issue happening ?

-Emil