[RFCv1,3/4] toolchain: post-pone evaluation of TOOLCHAIN_EXTERNAL_BIN

Message ID 20171103160627.6468-4-thomas.petazzoni@free-electrons.com
State New
Headers show
Series
  • Per-package SDK and target directories
Related show

Commit Message

Thomas Petazzoni Nov. 3, 2017, 4:06 p.m.
The upcoming per-package SDK functionality is heavily based on the
fact that HOST_DIR, STAGING_DIR and TARGET_DIR are evaluated during
the configure/build/install steps of the packages. Therefore, any
evaluation-during-assignment using := is going to cause problems, and
need to be turned into evaluation-during-use using =.

This patch fix up one such instance in the external toolchain code.

This change is independent from the per-package SDK functionality, and
could be applied separately.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 toolchain/toolchain-external/pkg-toolchain-external.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Arnout Vandecappelle Nov. 7, 2017, 9:23 p.m. | #1
On 03-11-17 17:06, Thomas Petazzoni wrote:
> The upcoming per-package SDK functionality is heavily based on the
> fact that HOST_DIR, STAGING_DIR and TARGET_DIR are evaluated during
> the configure/build/install steps of the packages. Therefore, any
> evaluation-during-assignment using := is going to cause problems, and
> need to be turned into evaluation-during-use using =.
> 
> This patch fix up one such instance in the external toolchain code.
             fixes

> 
> This change is independent from the per-package SDK functionality, and
> could be applied separately.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  toolchain/toolchain-external/pkg-toolchain-external.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/toolchain/toolchain-external/pkg-toolchain-external.mk b/toolchain/toolchain-external/pkg-toolchain-external.mk
> index dc0588c536..b9ad1720a1 100644
> --- a/toolchain/toolchain-external/pkg-toolchain-external.mk
> +++ b/toolchain/toolchain-external/pkg-toolchain-external.mk
> @@ -77,7 +77,7 @@ ifneq ($(TOOLCHAIN_EXTERNAL_PREFIX),)
>  TOOLCHAIN_EXTERNAL_BIN := $(dir $(shell which $(TOOLCHAIN_EXTERNAL_PREFIX)-gcc))
>  endif
>  else
> -TOOLCHAIN_EXTERNAL_BIN := $(TOOLCHAIN_EXTERNAL_INSTALL_DIR)/bin
> +TOOLCHAIN_EXTERNAL_BIN = $(TOOLCHAIN_EXTERNAL_INSTALL_DIR)/bin

 It gives me the shivers to see the same variable sometimes defined recursive
and sometimes defined immediate... But I can't find an elegant solution (other
than introducing an artificial extra variable, which is also ugly), so

Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

 Regards,
 Arnout


>  endif
>  
>  # If this is a buildroot toolchain, it already has a wrapper which we want to
>
Thomas Petazzoni Nov. 7, 2017, 9:33 p.m. | #2
Hello,

On Tue, 7 Nov 2017 22:23:24 +0100, Arnout Vandecappelle wrote:

> > diff --git a/toolchain/toolchain-external/pkg-toolchain-external.mk b/toolchain/toolchain-external/pkg-toolchain-external.mk
> > index dc0588c536..b9ad1720a1 100644
> > --- a/toolchain/toolchain-external/pkg-toolchain-external.mk
> > +++ b/toolchain/toolchain-external/pkg-toolchain-external.mk
> > @@ -77,7 +77,7 @@ ifneq ($(TOOLCHAIN_EXTERNAL_PREFIX),)
> >  TOOLCHAIN_EXTERNAL_BIN := $(dir $(shell which $(TOOLCHAIN_EXTERNAL_PREFIX)-gcc))
> >  endif
> >  else
> > -TOOLCHAIN_EXTERNAL_BIN := $(TOOLCHAIN_EXTERNAL_INSTALL_DIR)/bin
> > +TOOLCHAIN_EXTERNAL_BIN = $(TOOLCHAIN_EXTERNAL_INSTALL_DIR)/bin  
> 
>  It gives me the shivers to see the same variable sometimes defined recursive
> and sometimes defined immediate... But I can't find an elegant solution (other
> than introducing an artificial extra variable, which is also ugly), so
> 
> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

I also saw the other TOOLCHAIN_EXTERNAL_BIN assignment using := above,
and didn't change it for now because that was not the case I was
interested in testing/fixing. But my plan was to get back to this :=
assignment at some point. However, it seems like you have concluded
that we have to keep := here. Could you explain why?

Note: I think keeping the := is OK, because TOOLCHAIN_EXTERNAL_PREFIX
doesn't reference anything like HOST_DIR/STAGING_DIR/TARGET_DIR, so
it's fine to have evaluation at the time of assignment for this case.

Perhaps a comment above would help clarify that it's intentional?

Thomas
Arnout Vandecappelle Nov. 7, 2017, 11:49 p.m. | #3
On 07-11-17 22:33, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 7 Nov 2017 22:23:24 +0100, Arnout Vandecappelle wrote:
> 
>>> diff --git a/toolchain/toolchain-external/pkg-toolchain-external.mk b/toolchain/toolchain-external/pkg-toolchain-external.mk
>>> index dc0588c536..b9ad1720a1 100644
>>> --- a/toolchain/toolchain-external/pkg-toolchain-external.mk
>>> +++ b/toolchain/toolchain-external/pkg-toolchain-external.mk
>>> @@ -77,7 +77,7 @@ ifneq ($(TOOLCHAIN_EXTERNAL_PREFIX),)
>>>  TOOLCHAIN_EXTERNAL_BIN := $(dir $(shell which $(TOOLCHAIN_EXTERNAL_PREFIX)-gcc))
>>>  endif
>>>  else
>>> -TOOLCHAIN_EXTERNAL_BIN := $(TOOLCHAIN_EXTERNAL_INSTALL_DIR)/bin
>>> +TOOLCHAIN_EXTERNAL_BIN = $(TOOLCHAIN_EXTERNAL_INSTALL_DIR)/bin  
>>
>>  It gives me the shivers to see the same variable sometimes defined recursive
>> and sometimes defined immediate... But I can't find an elegant solution (other
>> than introducing an artificial extra variable, which is also ugly), so
>>
>> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> 
> I also saw the other TOOLCHAIN_EXTERNAL_BIN assignment using := above,
> and didn't change it for now because that was not the case I was
> interested in testing/fixing. But my plan was to get back to this :=
> assignment at some point. However, it seems like you have concluded
> that we have to keep := here. Could you explain why?

 It's using $(shell which ...). That variable ends up in $(TARGET_CC), so it is
re-evaluated hundreds of times. I don't know if it really makes much of a
difference, but the rule of thumb is: think twice about using
recursively-expanded if it's used in many places and it contains $(shell ...)
constructs.

> 
> Note: I think keeping the := is OK, because TOOLCHAIN_EXTERNAL_PREFIX
> doesn't reference anything like HOST_DIR/STAGING_DIR/TARGET_DIR, so
> it's fine to have evaluation at the time of assignment for this case.
> 
> Perhaps a comment above would help clarify that it's intentional?

 Not really needed, the code speaks for itself. The $(shell) makes it quite easy
to see why immediate expansion is chosen there.

 Regards,
 Arnout

Patch

diff --git a/toolchain/toolchain-external/pkg-toolchain-external.mk b/toolchain/toolchain-external/pkg-toolchain-external.mk
index dc0588c536..b9ad1720a1 100644
--- a/toolchain/toolchain-external/pkg-toolchain-external.mk
+++ b/toolchain/toolchain-external/pkg-toolchain-external.mk
@@ -77,7 +77,7 @@  ifneq ($(TOOLCHAIN_EXTERNAL_PREFIX),)
 TOOLCHAIN_EXTERNAL_BIN := $(dir $(shell which $(TOOLCHAIN_EXTERNAL_PREFIX)-gcc))
 endif
 else
-TOOLCHAIN_EXTERNAL_BIN := $(TOOLCHAIN_EXTERNAL_INSTALL_DIR)/bin
+TOOLCHAIN_EXTERNAL_BIN = $(TOOLCHAIN_EXTERNAL_INSTALL_DIR)/bin
 endif
 
 # If this is a buildroot toolchain, it already has a wrapper which we want to