| Message ID | 20220925090920.1585682-3-thomas.petazzoni@bootlin.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fixes/improvements in Xtensa handling | expand |
Thomas, All, On 2022-09-25 11:09 +0200, Thomas Petazzoni spake thusly: > This reverts commit 4cbf7336914f25478aea943456ba7dc3c892c21a. > > This commit breaks the build of BR2_XTENSA_CUSTOM=y configurations > that use an external toolchain. In such configurations, having an > empty overlay is perfectly fine. As 4cbf7336914f explained, an empty overlay is equivalent to using the fsf variant, so in that case, there would be no point in asking for a custom core to begin with, and Max Filipov seemed to agree: https://lore.kernel.org/buildroot/CAMo8BfKHKuVC86uOwP0z8CzE425q7u0B_q6jdeU0Rw9upaiY=Q@mail.gmail.com/ So, I think we are maybe taking the issue from the wrong side here. We should forbid an empty overlay only when using our internal toolchain for a custom xtensa core. I.e.: diff --git a/arch/arch.mk.xtensa b/arch/arch.mk.xtensa index 7b6c59cecd..1799528a93 100644 --- a/arch/arch.mk.xtensa +++ b/arch/arch.mk.xtensa @@ -1,8 +1,10 @@ BR_ARCH_XTENSA_OVERLAY_FILE = $(call qstrip,$(BR2_XTENSA_OVERLAY_FILE)) -ifeq ($(BR_BUILDING)$(BR2_XTENSA_CUSTOM):$(BR_ARCH_XTENSA_OVERLAY_FILE),yy:) +ifeq ($(BR_BUILDING)$(BR2_TOOLCHAIN_BUILDROOT),yy) +ifeq ($(BR_ARCH_XTENSA_OVERLAY_FILE),) $(error No xtensa overlay file provided. Check your BR2_XTENSA_OVERLAY_FILE setting) endif +endif ################################################################################ # This variable can be used by packages that need to extract the # overlay. Regards, Yann E. MORIN. > For example, this commit broke the > following two runtime tests: > > tests.toolchain.test_external_bootlin.TestExternalToolchainBootlinXtensalx60UclibcBleedingEdge > tests.toolchain.test_external_bootlin.TestExternalToolchainBootlinXtensalx60UclibcStable > > And in fact, having an empty overlay is even correct when building an > internal toolchain. The endianness issue that > 4cbf7336914f25478aea943456ba7dc3c892c21a was attempting to fix has > been fixed in a better way by the previous commit. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > --- > arch/arch.mk.xtensa | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/arch/arch.mk.xtensa b/arch/arch.mk.xtensa > index 7b6c59cecd..fd410f6bfa 100644 > --- a/arch/arch.mk.xtensa > +++ b/arch/arch.mk.xtensa > @@ -1,9 +1,3 @@ > -BR_ARCH_XTENSA_OVERLAY_FILE = $(call qstrip,$(BR2_XTENSA_OVERLAY_FILE)) > - > -ifeq ($(BR_BUILDING)$(BR2_XTENSA_CUSTOM):$(BR_ARCH_XTENSA_OVERLAY_FILE),yy:) > -$(error No xtensa overlay file provided. Check your BR2_XTENSA_OVERLAY_FILE setting) > -endif > - > ################################################################################ > # This variable can be used by packages that need to extract the overlay. > # > @@ -15,6 +9,7 @@ endif > # tar xf $(ARCH_XTENSA_OVERLAY_FILE) -C $(@D) --strip-components=1 gcc > # endif > ################################################################################ > +BR_ARCH_XTENSA_OVERLAY_FILE = $(call qstrip,$(BR2_XTENSA_OVERLAY_FILE)) > ifneq ($(filter http://% https://% ftp://% scp://%,$(BR_ARCH_XTENSA_OVERLAY_FILE)),) > ARCH_XTENSA_OVERLAY_URL = $(BR_ARCH_XTENSA_OVERLAY_FILE) > ARCH_XTENSA_OVERLAY_FILE = $($(PKG)_DL_DIR)/$(notdir $(BR_ARCH_XTENSA_OVERLAY_FILE)) > -- > 2.37.3 >
Hello, On Sun, 25 Sep 2022 12:03:55 +0200 "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > As 4cbf7336914f explained, an empty overlay is equivalent to using the > fsf variant, so in that case, there would be no point in asking for a > custom core to begin with, and Max Filipov seemed to agree: > https://lore.kernel.org/buildroot/CAMo8BfKHKuVC86uOwP0z8CzE425q7u0B_q6jdeU0Rw9upaiY=Q@mail.gmail.com/ > > So, I think we are maybe taking the issue from the wrong side here. We > should forbid an empty overlay only when using our internal toolchain > for a custom xtensa core. I.e.: > > diff --git a/arch/arch.mk.xtensa b/arch/arch.mk.xtensa > index 7b6c59cecd..1799528a93 100644 > --- a/arch/arch.mk.xtensa > +++ b/arch/arch.mk.xtensa > @@ -1,8 +1,10 @@ > BR_ARCH_XTENSA_OVERLAY_FILE = $(call qstrip,$(BR2_XTENSA_OVERLAY_FILE)) > > -ifeq ($(BR_BUILDING)$(BR2_XTENSA_CUSTOM):$(BR_ARCH_XTENSA_OVERLAY_FILE),yy:) > +ifeq ($(BR_BUILDING)$(BR2_TOOLCHAIN_BUILDROOT),yy) > +ifeq ($(BR_ARCH_XTENSA_OVERLAY_FILE),) > $(error No xtensa overlay file provided. Check your BR2_XTENSA_OVERLAY_FILE setting) > endif > +endif You're right, this seems to be a better approach. In the case of an internal toolchain with custom core, it means the user still sees the little/big endian choice, but if he hasn't specified an overlay, the build will be aborted early on. I guess I'm on for a v3! Thomas
On Sun, 25 Sep 2022 12:13:02 +0200 Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > > As 4cbf7336914f explained, an empty overlay is equivalent to using the > > fsf variant, so in that case, there would be no point in asking for a > > custom core to begin with, and Max Filipov seemed to agree: > > https://lore.kernel.org/buildroot/CAMo8BfKHKuVC86uOwP0z8CzE425q7u0B_q6jdeU0Rw9upaiY=Q@mail.gmail.com/ > > > > So, I think we are maybe taking the issue from the wrong side here. We > > should forbid an empty overlay only when using our internal toolchain > > for a custom xtensa core. I.e.: > > > > diff --git a/arch/arch.mk.xtensa b/arch/arch.mk.xtensa > > index 7b6c59cecd..1799528a93 100644 > > --- a/arch/arch.mk.xtensa > > +++ b/arch/arch.mk.xtensa > > @@ -1,8 +1,10 @@ > > BR_ARCH_XTENSA_OVERLAY_FILE = $(call qstrip,$(BR2_XTENSA_OVERLAY_FILE)) > > > > -ifeq ($(BR_BUILDING)$(BR2_XTENSA_CUSTOM):$(BR_ARCH_XTENSA_OVERLAY_FILE),yy:) > > +ifeq ($(BR_BUILDING)$(BR2_TOOLCHAIN_BUILDROOT),yy) > > +ifeq ($(BR_ARCH_XTENSA_OVERLAY_FILE),) > > $(error No xtensa overlay file provided. Check your BR2_XTENSA_OVERLAY_FILE setting) > > endif > > +endif > > You're right, this seems to be a better approach. In the case of an > internal toolchain with custom core, it means the user still sees the > little/big endian choice, but if he hasn't specified an overlay, the > build will be aborted early on. Now that I re-read this, I'm no longer sure to understand. If you have a configuration BR2_TOOLCHAIN_BUILDROOT=y, BR2_xtensa_fsf=y, then BR2_XTENSA_OVERLAY_FILE will be empty, but it's perfectly normal. But with your proposed change, such a configuration would no longer build, as we would now always require an overlay to be specified. Best regards, Thomas
diff --git a/arch/arch.mk.xtensa b/arch/arch.mk.xtensa index 7b6c59cecd..fd410f6bfa 100644 --- a/arch/arch.mk.xtensa +++ b/arch/arch.mk.xtensa @@ -1,9 +1,3 @@ -BR_ARCH_XTENSA_OVERLAY_FILE = $(call qstrip,$(BR2_XTENSA_OVERLAY_FILE)) - -ifeq ($(BR_BUILDING)$(BR2_XTENSA_CUSTOM):$(BR_ARCH_XTENSA_OVERLAY_FILE),yy:) -$(error No xtensa overlay file provided. Check your BR2_XTENSA_OVERLAY_FILE setting) -endif - ################################################################################ # This variable can be used by packages that need to extract the overlay. # @@ -15,6 +9,7 @@ endif # tar xf $(ARCH_XTENSA_OVERLAY_FILE) -C $(@D) --strip-components=1 gcc # endif ################################################################################ +BR_ARCH_XTENSA_OVERLAY_FILE = $(call qstrip,$(BR2_XTENSA_OVERLAY_FILE)) ifneq ($(filter http://% https://% ftp://% scp://%,$(BR_ARCH_XTENSA_OVERLAY_FILE)),) ARCH_XTENSA_OVERLAY_URL = $(BR_ARCH_XTENSA_OVERLAY_FILE) ARCH_XTENSA_OVERLAY_FILE = $($(PKG)_DL_DIR)/$(notdir $(BR_ARCH_XTENSA_OVERLAY_FILE))
This reverts commit 4cbf7336914f25478aea943456ba7dc3c892c21a. This commit breaks the build of BR2_XTENSA_CUSTOM=y configurations that use an external toolchain. In such configurations, having an empty overlay is perfectly fine. For example, this commit broke the following two runtime tests: tests.toolchain.test_external_bootlin.TestExternalToolchainBootlinXtensalx60UclibcBleedingEdge tests.toolchain.test_external_bootlin.TestExternalToolchainBootlinXtensalx60UclibcStable And in fact, having an empty overlay is even correct when building an internal toolchain. The endianness issue that 4cbf7336914f25478aea943456ba7dc3c892c21a was attempting to fix has been fixed in a better way by the previous commit. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> --- arch/arch.mk.xtensa | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)