Discussion:
[PATCH] android: amd/common: fix sid_tables.h generation rules
Add Reply
Mauro Rossi
2017-08-11 14:02:10 UTC
Reply
Permalink
Raw Message
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-***@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
Rob Herring
2017-08-11 14:23:14 UTC
Reply
Permalink
Raw Message
Post by Mauro Rossi
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.
Post by Mauro Rossi
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?
Post by Mauro Rossi
---
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)
-$(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
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Mauro Rossi
2017-08-11 15:10:24 UTC
Reply
Permalink
Raw Message
Post by Rob Herring
Post by Mauro Rossi
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.
Post by Mauro Rossi
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
Post by Rob Herring
Post by Mauro Rossi
---
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)
-$(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
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Rob Herring
2017-08-11 15:25:58 UTC
Reply
Permalink
Raw Message
Post by Mauro Rossi
Post by Rob Herring
Post by Mauro Rossi
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.
Post by Mauro Rossi
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.
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.
Post by Mauro Rossi
Mauro
PS: If it is a false positive and not needed in 17.2 and mesa-dev
please Rob, Chih-Wei just tell me,
+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
Mauro Rossi
2017-08-11 18:06:09 UTC
Reply
Permalink
Raw Message
Post by Rob Herring
Post by Mauro Rossi
Post by Rob Herring
Post by Mauro Rossi
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.
Post by Mauro Rossi
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.
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/
Post by Rob Herring
Post by Mauro Rossi
Mauro
PS: If it is a false positive and not needed in 17.2 and mesa-dev
please Rob, Chih-Wei just tell me,
+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
Mauro Rossi
2017-08-12 06:58:52 UTC
Reply
Permalink
Raw Message
Post by Rob Herring
Post by Mauro Rossi
Post by Rob Herring
Post by Mauro Rossi
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.
Post by Mauro Rossi
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.
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.
Post by Rob Herring
Post by Mauro Rossi
Mauro
PS: If it is a false positive and not needed in 17.2 and mesa-dev
please Rob, Chih-Wei just tell me,
+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.

Loading...