[Mesa-dev] [PATCH] android: amd/common: fix sid_tables.h generation rules

[Mauro Rossi]

2017-08-11 17:25 GMT+02:00 Rob Herring <robh at kernel.org>:
> On Fri, Aug 11, 2017 at 10:10 AM, Mauro Rossi <issor.oruam at gmail.com> wrote:
>> 2017-08-11 16:23 GMT+02:00 Rob Herring <robh at kernel.org>:
>>> On Fri, Aug 11, 2017 at 9:02 AM, Mauro Rossi <issor.oruam at gmail.com> wrote:
>>>> Current generation rules rely on LOCAL_PATH variable,
>>>> which may be undefined when dependencies are expanded;
>>>> move to using MESA_TOP variable to define sid_tables.py script path
>>>
>>> I count roughly 67 occurrences of pointing to python scripts using
>>> LOCAL_PATH. Presumably they all need to be fixed or this isn't really
>>> the problem.
>>>
>>>> Fixes the following building error:
>>>>
>>>> external/mesa/src/gallium/drivers/radeonsi/si_debug.c:30:10: fatal error: 'sid_tables.h' file not found
>>>>          ^
>>>> 1 error generated.
>>>>
>>>> Fixes: 730574c58e "android: amd/common: add support for libmesa_amd_common"
>>>
>>> Why do I not see this error?
>>
>>
>> I was also suprised to see the error,
>> it started to appear persistently when building nougat-x86 from scratch.
>>
>> As a similar case I saw this one:
>> https://cgit.freedesktop.org/mesa/mesa/commit/?id=c1a29e104cc585ad3219b12d09f532a129d68dad
>>
>> and in general I empirically saw it is unsafe to use $(LOCAL_PATH) in
>> generated files dependencies rules.
>> Chih-Wei may know better the reason.
>
> The discussion on this concluded that LOCAL_PATH as a rule dependency
> is okay. LOCAL_PATH in the recipe for the rule is not.

But if I understand the discussion [1] correctly,

in the end we should never use $(LOCAL_PATH) in the dependencies of
generated sources,
without having defined private variable in the dependencies context
(which is not this case)

or the expansion is not guaranteed to work, yet it may succeed by pure
chance/by build reiteration,
but in my case (and also Tapani's) it was failing sistematically.

[1] https://patchwork.freedesktop.org/patch/167718/

>
>>
>> Added him in Cc:
>>
>> Mauro
>>
>> PS: If it is a false positive and not needed in 17.2 and mesa-dev
>> please Rob, Chih-Wei just tell me,
>> but in any case 17.1 branch requires to add:
>>
>> +LOCAL_STATIC_LIBRARIES := libmesa_amd_common
>>
>> in https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/drivers/radeonsi/Android.mk?h=17.1
>
> Not sure, but probably needed as radeonsi was not in good shape in 17.1.
>
> Rob