android: amd/common: fix sid_tables.h generation rules

Submitted by Mauro Rossi on Aug. 11, 2017, 2:02 p.m.

Details

Message ID 20170811140210.12208-1-issor.oruam@gmail.com
State New
Headers show
Series "android: amd/common: fix sid_tables.h generation rules" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Mauro Rossi Aug. 11, 2017, 2:02 p.m.
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

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"

Cc: "17.1 17.2" <mesa-stable@lists.freedesktop.org>
---
 src/amd/Android.common.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/amd/Android.common.mk b/src/amd/Android.common.mk
index 7d08bfd31d..f4497ed639 100644
--- a/src/amd/Android.common.mk
+++ b/src/amd/Android.common.mk
@@ -44,7 +44,7 @@  LOCAL_GENERATED_SOURCES := $(addprefix $(intermediates)/, $(AMD_GENERATED_FILES)
 $(LOCAL_GENERATED_SOURCES): PRIVATE_PYTHON := $(MESA_PYTHON2)
 $(LOCAL_GENERATED_SOURCES): PRIVATE_CUSTOM_TOOL = $(PRIVATE_PYTHON) $^ > $@
 
-$(intermediates)/common/sid_tables.h: $(LOCAL_PATH)/common/sid_tables.py $(MESA_TOP)/src/amd/common/sid.h
+$(intermediates)/common/sid_tables.h: $(MESA_TOP)/src/amd/common/sid_tables.py $(MESA_TOP)/src/amd/common/sid.h
 	$(transform-generated-source)
 
 LOCAL_C_INCLUDES := \

Comments

On Fri, Aug 11, 2017 at 9:02 AM, Mauro Rossi <issor.oruam@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?

>
> Cc: "17.1 17.2" <mesa-stable@lists.freedesktop.org>
> ---
>  src/amd/Android.common.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/amd/Android.common.mk b/src/amd/Android.common.mk
> index 7d08bfd31d..f4497ed639 100644
> --- a/src/amd/Android.common.mk
> +++ b/src/amd/Android.common.mk
> @@ -44,7 +44,7 @@ LOCAL_GENERATED_SOURCES := $(addprefix $(intermediates)/, $(AMD_GENERATED_FILES)
>  $(LOCAL_GENERATED_SOURCES): PRIVATE_PYTHON := $(MESA_PYTHON2)
>  $(LOCAL_GENERATED_SOURCES): PRIVATE_CUSTOM_TOOL = $(PRIVATE_PYTHON) $^ > $@
>
> -$(intermediates)/common/sid_tables.h: $(LOCAL_PATH)/common/sid_tables.py $(MESA_TOP)/src/amd/common/sid.h
> +$(intermediates)/common/sid_tables.h: $(MESA_TOP)/src/amd/common/sid_tables.py $(MESA_TOP)/src/amd/common/sid.h
>         $(transform-generated-source)
>
>  LOCAL_C_INCLUDES := \
> --
> 2.11.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
2017-08-11 16:23 GMT+02:00 Rob Herring <robh@kernel.org>:
> On Fri, Aug 11, 2017 at 9:02 AM, Mauro Rossi <issor.oruam@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.

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

>
>>
>> Cc: "17.1 17.2" <mesa-stable@lists.freedesktop.org>
>> ---
>>  src/amd/Android.common.mk | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/amd/Android.common.mk b/src/amd/Android.common.mk
>> index 7d08bfd31d..f4497ed639 100644
>> --- a/src/amd/Android.common.mk
>> +++ b/src/amd/Android.common.mk
>> @@ -44,7 +44,7 @@ LOCAL_GENERATED_SOURCES := $(addprefix $(intermediates)/, $(AMD_GENERATED_FILES)
>>  $(LOCAL_GENERATED_SOURCES): PRIVATE_PYTHON := $(MESA_PYTHON2)
>>  $(LOCAL_GENERATED_SOURCES): PRIVATE_CUSTOM_TOOL = $(PRIVATE_PYTHON) $^ > $@
>>
>> -$(intermediates)/common/sid_tables.h: $(LOCAL_PATH)/common/sid_tables.py $(MESA_TOP)/src/amd/common/sid.h
>> +$(intermediates)/common/sid_tables.h: $(MESA_TOP)/src/amd/common/sid_tables.py $(MESA_TOP)/src/amd/common/sid.h
>>         $(transform-generated-source)
>>
>>  LOCAL_C_INCLUDES := \
>> --
>> 2.11.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Fri, Aug 11, 2017 at 10:10 AM, Mauro Rossi <issor.oruam@gmail.com> wrote:
> 2017-08-11 16:23 GMT+02:00 Rob Herring <robh@kernel.org>:
>> On Fri, Aug 11, 2017 at 9:02 AM, Mauro Rossi <issor.oruam@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.

>
> 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
2017-08-11 17:25 GMT+02:00 Rob Herring <robh@kernel.org>:
> On Fri, Aug 11, 2017 at 10:10 AM, Mauro Rossi <issor.oruam@gmail.com> wrote:
>> 2017-08-11 16:23 GMT+02:00 Rob Herring <robh@kernel.org>:
>>> On Fri, Aug 11, 2017 at 9:02 AM, Mauro Rossi <issor.oruam@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
2017-08-11 20:06 GMT+02:00 Mauro Rossi <issor.oruam@gmail.com>:
> 2017-08-11 17:25 GMT+02:00 Rob Herring <robh@kernel.org>:
>> On Fri, Aug 11, 2017 at 10:10 AM, Mauro Rossi <issor.oruam@gmail.com> wrote:
>>> 2017-08-11 16:23 GMT+02:00 Rob Herring <robh@kernel.org>:
>>>> On Fri, Aug 11, 2017 at 9:02 AM, Mauro Rossi <issor.oruam@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.
>

Hi Rob,

It is as you said, the patch is not necessary

Sorry, I suspect I've been affected by a failing hard drive
M.

>
>>
>>>
>>> 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

Now I cross-checked and the additional static dependendency is not needed either
Also mesa-stable can be dropped and does not need changes
M.