diff mbox

igt: tests/Android.mk: fix LOCAL_PATH usage

Message ID 1389722498-25473-1-git-send-email-robert.beckett@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

robert.beckett@intel.com Jan. 14, 2014, 6:01 p.m. UTC 
From: Robert Beckett <robert.beckett@intel.com>

Fix usage of shared variable LOCAL_PATH in deferred variable expansion area.

Signed-off-by: Robert Beckett <robert.beckett@intel.com>
---
 tests/Android.mk |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Lespiau, Damien Jan. 14, 2014, 5:57 p.m. UTC | #1 
On Tue, Jan 14, 2014 at 06:01:38PM +0000, robert.beckett@intel.com wrote:
> From: Robert Beckett <robert.beckett@intel.com>
> 
> Fix usage of shared variable LOCAL_PATH in deferred variable expansion area.

Out of curiosity, do you mind explaining what the late evaluation of
LOCAL_PATH broke? and why you didn't need to update the
$(LOCAL_PATH)/version.h targets?

Thanks,
robert.beckett@intel.com Jan. 14, 2014, 6:29 p.m. UTC | #2 
In Makefile language, rule and dependency definitions use immediate expansions of variables, so they get expanded as soon as the rule is created (1st pass). Rule implementation (a.k.a recipe) use deferred expansion (2nd pass).

Android effectively makes all Android.mk files a single makefile by including them all in a big tree from the toplevel makefile. The rules are all evaluated in the first pass and targets are generated. Then the 2nd pass happens and the required target's recipes are run. At this point, LOCAL_PATH has been assigned the value from the last evaluated Android.mk in the 1st phase that defined LOCAL_PATH (most Android.mk use this variable). In my particular case, it was the bootloader's Android.mk that was evaluated last and had defined LOCAL_PATH to it's path. The errors are rather misleading due to it looking like a bug in another module's Android.mk rather than this one :)

Basically, if you want to use a variable that any other Android.mk defines, then you can only use it in an immediate expansion context, not a deferred expansion context as it will likely be re-defined by the time the 2nd pass happens.

This patch stores it in a unique variable that should not be being used by other Android.mk files. An alternative fix would be to use $@ and $< as the files in question are target and dependency, but I never like using those as they can easily break if dependencies are added etc. I prefer variable to be explicitly named to make them obvious.

(see gnu make manual for explanation of deferred vs immediate expansion of variables : http://www.gnu.org/software/make/manual/make.html )

-----Original Message-----
From: Lespiau, Damien 
Sent: Tuesday, January 14, 2014 5:57 PM
To: Beckett, Robert
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] igt: tests/Android.mk: fix LOCAL_PATH usage

On Tue, Jan 14, 2014 at 06:01:38PM +0000, robert.beckett@intel.com wrote:
> From: Robert Beckett <robert.beckett@intel.com>
> 
> Fix usage of shared variable LOCAL_PATH in deferred variable expansion area.

Out of curiosity, do you mind explaining what the late evaluation of LOCAL_PATH broke? and why you didn't need to update the $(LOCAL_PATH)/version.h targets?

Thanks,

--
Damien

> Signed-off-by: Robert Beckett <robert.beckett@intel.com>
> ---
>  tests/Android.mk |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/Android.mk b/tests/Android.mk index 
> c65f94c..abccb7f 100644
> --- a/tests/Android.mk
> +++ b/tests/Android.mk
> @@ -22,6 +22,7 @@ skip_lib_list := \
>  
>  lib_list := $(filter-out 
> $(skip_lib_list),$(libintel_tools_la_SOURCES))
>  LIB_SOURCES := $(addprefix lib/,$(lib_list))
> +GPU_TOOLS_PATH := $(LOCAL_PATH)
>  
>  .PHONY: version.h.tmp
>  
> @@ -38,10 +39,10 @@ $(LOCAL_PATH)/version.h.tmp:
>  
>  $(LOCAL_PATH)/version.h: $(LOCAL_PATH)/version.h.tmp
>  	@echo "updating version.h"
> -	@if ! cmp -s $(LOCAL_PATH)/version.h.tmp $(LOCAL_PATH)/version.h; then \
> -		mv $(LOCAL_PATH)/version.h.tmp $(LOCAL_PATH)/version.h ; \
> +	@if ! cmp -s $(GPU_TOOLS_PATH)/version.h.tmp $(GPU_TOOLS_PATH)/version.h; then \
> +		mv $(GPU_TOOLS_PATH)/version.h.tmp $(GPU_TOOLS_PATH)/version.h ; \
>  	else \
> -		rm $(LOCAL_PATH)/version.h.tmp ; \
> +		rm $(GPU_TOOLS_PATH)/version.h.tmp ; \
>  	fi
>  
>  # FIXME: autogenerate this info #
> --
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Lespiau, Damien Jan. 14, 2014, 6:53 p.m. UTC | #3 
On Tue, Jan 14, 2014 at 06:29:48PM +0000, Beckett, Robert wrote:
> In Makefile language, rule and dependency definitions use immediate
> expansions of variables, so they get expanded as soon as the rule is
> created (1st pass). Rule implementation (a.k.a recipe) use deferred
> expansion (2nd pass).

I had forgotten that the rules are expanded in the first pass, makes it
clear. Thanks.

Pushed, with this blob attached and a direct link the "how make reads a
Makefile section", that makes a much better commit message!
diff mbox

Patch

diff --git a/tests/Android.mk b/tests/Android.mk
index c65f94c..abccb7f 100644
--- a/tests/Android.mk
+++ b/tests/Android.mk
@@ -22,6 +22,7 @@  skip_lib_list := \
 
 lib_list := $(filter-out $(skip_lib_list),$(libintel_tools_la_SOURCES))
 LIB_SOURCES := $(addprefix lib/,$(lib_list))
+GPU_TOOLS_PATH := $(LOCAL_PATH)
 
 .PHONY: version.h.tmp
 
@@ -38,10 +39,10 @@  $(LOCAL_PATH)/version.h.tmp:
 
 $(LOCAL_PATH)/version.h: $(LOCAL_PATH)/version.h.tmp
 	@echo "updating version.h"
-	@if ! cmp -s $(LOCAL_PATH)/version.h.tmp $(LOCAL_PATH)/version.h; then \
-		mv $(LOCAL_PATH)/version.h.tmp $(LOCAL_PATH)/version.h ; \
+	@if ! cmp -s $(GPU_TOOLS_PATH)/version.h.tmp $(GPU_TOOLS_PATH)/version.h; then \
+		mv $(GPU_TOOLS_PATH)/version.h.tmp $(GPU_TOOLS_PATH)/version.h ; \
 	else \
-		rm $(LOCAL_PATH)/version.h.tmp ; \
+		rm $(GPU_TOOLS_PATH)/version.h.tmp ; \
 	fi
 
 # FIXME: autogenerate this info #