| Message ID | 20250805171619.1357193-1-raj.khem@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | machine/arch-arm: Append TUNE_CCARGS_MARCH to TUNE_CCARGS conditionally | expand |
The patch below corrects the error we were seeing in our CI. Thanks, Jon From: Khem Raj <raj.khem@gmail.com> Date: Tuesday, August 5, 2025 at 1:16 PM To: openembedded-core@lists.openembedded.org <openembedded-core@lists.openembedded.org> Cc: Khem Raj <raj.khem@gmail.com>, Jon Mason <Jon.Mason@arm.com>, Ryan Eatmon <reatmon@ti.com> Subject: [PATCH] machine/arch-arm: Append TUNE_CCARGS_MARCH to TUNE_CCARGS conditionally With [1], we now have a non-empty else block for arm-features, uptil now feature includes only appended when feature was set and unset case was appending empty string, which hid this issue where TUNE_CCARGS_MARCH_OPTS could be non-empty but TUNE_CCARGS_MARCH is empty, this is possible when a tune file overwrites TUNE_FEATURES and does not have a feature that would add -march or -mcpu at all. This changeset adds a check to not append at all if TUNE_CCARGS_MARCH is unset. [1] https://git.openembedded.org/openembedded-core/commit/?id=db1b355b2b15ba57bd89c2dfb88c2c667551863e Signed-off-by: Khem Raj <raj.khem@gmail.com> Cc: Jon Mason <jon.mason@arm.com> Cc: Ryan Eatmon <reatmon@ti.com> --- meta/conf/machine/include/arm/arch-arm.inc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meta/conf/machine/include/arm/arch-arm.inc b/meta/conf/machine/include/arm/arch-arm.inc index f1e92e19c61..ae661b1ab4f 100644 --- a/meta/conf/machine/include/arm/arch-arm.inc +++ b/meta/conf/machine/include/arm/arch-arm.inc @@ -17,4 +17,4 @@ TARGET_FPU = "${@d.getVar('TUNE_CCARGS_MFLOAT') or 'soft'}" # Some -march settings need a +X option passed in. Since we cannot guarantee that any specified TUNE_CCARGS option is set in any order, we must hard code the order here to allow for it. TUNE_CCARGS_MARCH_OPTS ??= "" -TUNE_CCARGS .= "${TUNE_CCARGS_MARCH}${TUNE_CCARGS_MARCH_OPTS}" +TUNE_CCARGS .= "${@'${TUNE_CCARGS_MARCH}${TUNE_CCARGS_MARCH_OPTS}' if d.getVar('TUNE_CCARGS_MARCH') else ''}"
Fixes ours as well. Please get this out to master ASAP. On 8/5/2025 1:55 PM, Jon Mason wrote: > The patch below corrects the error we were seeing in our CI. > > Thanks, > > Jon > > *From: *Khem Raj <raj.khem@gmail.com> > *Date: *Tuesday, August 5, 2025 at 1:16 PM > *To: *openembedded-core@lists.openembedded.org > <openembedded-core@lists.openembedded.org> > *Cc: *Khem Raj <raj.khem@gmail.com>, Jon Mason <Jon.Mason@arm.com>, Ryan > Eatmon <reatmon@ti.com> > *Subject: *[PATCH] machine/arch-arm: Append TUNE_CCARGS_MARCH to > TUNE_CCARGS conditionally > > With [1], we now have a non-empty else block for arm-features, uptil now > feature includes only appended when feature was set and unset case was > appending empty string, which hid this issue where TUNE_CCARGS_MARCH_OPTS > could be non-empty but TUNE_CCARGS_MARCH is empty, this is possible when > a tune file overwrites TUNE_FEATURES and does not have a feature that would > add -march or -mcpu at all. > > This changeset adds a check to not append at all if TUNE_CCARGS_MARCH > is unset. > > [1] > https://git.openembedded.org/openembedded-core/commit/?id=db1b355b2b15ba57bd89c2dfb88c2c667551863e <https://git.openembedded.org/openembedded-core/commit/?id=db1b355b2b15ba57bd89c2dfb88c2c667551863e> > > Signed-off-by: Khem Raj <raj.khem@gmail.com> > Cc: Jon Mason <jon.mason@arm.com> > Cc: Ryan Eatmon <reatmon@ti.com> > --- > meta/conf/machine/include/arm/arch-arm.inc | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/meta/conf/machine/include/arm/arch-arm.inc > b/meta/conf/machine/include/arm/arch-arm.inc > index f1e92e19c61..ae661b1ab4f 100644 > --- a/meta/conf/machine/include/arm/arch-arm.inc > +++ b/meta/conf/machine/include/arm/arch-arm.inc > @@ -17,4 +17,4 @@ TARGET_FPU = "${@d.getVar('TUNE_CCARGS_MFLOAT') or > 'soft'}" > > # Some -march settings need a +X option passed in. Since we cannot > guarantee that any specified TUNE_CCARGS option is set in any order, we > must hard code the order here to allow for it. > TUNE_CCARGS_MARCH_OPTS ??= "" > -TUNE_CCARGS .= "${TUNE_CCARGS_MARCH}${TUNE_CCARGS_MARCH_OPTS}" > +TUNE_CCARGS .= "${@'${TUNE_CCARGS_MARCH}${TUNE_CCARGS_MARCH_OPTS}' if > d.getVar('TUNE_CCARGS_MARCH') else ''}" >
On Tue, 2025-08-05 at 18:55 +0000, Jon Mason via lists.openembedded.org wrote:
> The patch below corrects the error we were seeing in our CI.
I think I need to step in here.
Just because a patch fixes a regression, it doesn't always follow it is
the right thing to do.
I'm worried we're creating a mess of various "rules" about what can and
can't go into variables and we're going to end up pinning ourselves
into something unmaintainable.
Yes, the tune files have some complex "rules" already which need to be
followed when changing them or writing new ones but I'm not convinced
this patch improves the situation, I think the opposite, it adds more
rules (most of which aren't documented).
As such I think I'd favour a revert of the original patch until this
gets fixed in a way which doesn't involve changing every tune file and
inline conditional python. I appreciate the original patch was needed
for clang issues but that doesn't mean we can't find a better solution.
I need people to help step up and maintain things like this but it
involves thinking about the overall structure, not just that a given
patch patches up some CI jobs.
Cheers,
Richard
diff --git a/meta/conf/machine/include/arm/arch-arm.inc b/meta/conf/machine/include/arm/arch-arm.inc index f1e92e19c61..ae661b1ab4f 100644 --- a/meta/conf/machine/include/arm/arch-arm.inc +++ b/meta/conf/machine/include/arm/arch-arm.inc @@ -17,4 +17,4 @@ TARGET_FPU = "${@d.getVar('TUNE_CCARGS_MFLOAT') or 'soft'}" # Some -march settings need a +X option passed in. Since we cannot guarantee that any specified TUNE_CCARGS option is set in any order, we must hard code the order here to allow for it. TUNE_CCARGS_MARCH_OPTS ??= "" -TUNE_CCARGS .= "${TUNE_CCARGS_MARCH}${TUNE_CCARGS_MARCH_OPTS}" +TUNE_CCARGS .= "${@'${TUNE_CCARGS_MARCH}${TUNE_CCARGS_MARCH_OPTS}' if d.getVar('TUNE_CCARGS_MARCH') else ''}"
With [1], we now have a non-empty else block for arm-features, uptil now feature includes only appended when feature was set and unset case was appending empty string, which hid this issue where TUNE_CCARGS_MARCH_OPTS could be non-empty but TUNE_CCARGS_MARCH is empty, this is possible when a tune file overwrites TUNE_FEATURES and does not have a feature that would add -march or -mcpu at all. This changeset adds a check to not append at all if TUNE_CCARGS_MARCH is unset. [1] https://git.openembedded.org/openembedded-core/commit/?id=db1b355b2b15ba57bd89c2dfb88c2c667551863e Signed-off-by: Khem Raj <raj.khem@gmail.com> Cc: Jon Mason <jon.mason@arm.com> Cc: Ryan Eatmon <reatmon@ti.com> --- meta/conf/machine/include/arm/arch-arm.inc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)