| Message ID | 20171103160627.6468-4-thomas.petazzoni@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Per-package SDK and target directories | expand |
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 >
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
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
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
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(-)