Bug 13469

Summary: uboot-extlinux-config uses wrong description
Product: [Build System, Metadata & Runtime] OE-Core Reporter: Will Page <wpage>
Component: oe-core otherAssignee: Will Page <wpage>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: Medium CC: randy.macleod
Version: unspecified   
Target Milestone: 3.1   
Hardware: Other   
OS: Multiple   
Whiteboard:
OS type for building Yocto: --- Type of Regression: ---
Verified: Documentation change: Don't know

Description Will Page 2019-08-06 16:56:18 UTC
U-Boot's extlinux.conf label generation facilities (defined in meta/classes/uboot-extlinux-config.bbclass) generate incorrect results under some circumstances.

When using non-default labels (e.g. UBOOT_EXTLINUX_LABELS = "default", replacing the default value of "linux"), user-supplied label descriptions are ignored (e.g. UBOOT_EXTLINUX_MENU_DESCRIPTION_default = "Foo" will always be ignored in favor of the default value supplied in UBOOT_EXTLINUX_MENU_DESCRIPTION).

This behavior results from the class using OVERRIDES to support label-specific definitions, and the default label using an identifier that corresponds to a common ${OS} value, which by default appears in OVERRIDES.

For example:

UBOOT_EXTLINUX_LABELS = "default"
UBOOT_EXTLINUX_MENU_DESCRIPTION = "Foo"

During processing of the "default" label, the OVERRIDES will be updated from "${OS}:..." to "default:${OS}:...".  Because "linux" will be the most common value for ${OS} in this situation, it will frequently be evaluated as "default:linux:...".

Later, when writing out the entries for that label, it will request the value of "UBOOT_EXTLINUX_MENU_DESCRIPTION", at which point it will iterate through the OVERRIDES list, first processing the value for UBOOT_EXTLINUX_MENU_DESCRIPTION_default, then in a later iteration finding UBOOT_EXTLINUX_MENU_DESCRIPTION_linux, and using that value instead.

The OVERRIDE precedence rules essentially guarantee that the default value assigned to the _linux variable will supercede any user-supplied values for any and all labels.

I see three possible fixes:
1. change the provided label from "linux" to "default" or "primary" or similar, something that's not a commonly used value in OVERRIDES.  If using this solution, documentation changes should probably be made to discourage using OS-, DISTRO-, or MACHINE-related values for labels.
2. in the local copy of the dictionary used for generating label entries, clear out all pre-existing values of OVERRIDES
3. instead of prepending the label to the list of OVERRIDES, append the label to the list, giving it the highest precedence (although properly speaking it should be inserted before forcevariable, if it exists, giving it the second-highest priority).

A related issue exists where values defined for the first label shadow values defined for subsequent labels (when multiple labels are defined), because the dictionary gets re-used between iterations over the labels, and the newly-added label OVERRIDE has lower precedence than the previous labels, which are still present in the OVERRIDES list.

This related issue would be solved by options #2 and #3 above, although solution #2 is likely to result in surprising results where values not set for one label would essentially default to the value used for the previous label.

Alternatively, solution #1 combined with moving the dictionary copy into the loop, giving each label iteration a fresh copy of OVERRIDES (along with everything else) would also solve this issue.
Comment 1 Will Page 2019-08-06 17:33:29 UTC
I misspoke in my original description, where I said

"although solution #2 is likely to result in surprising results where values not set for one label would essentially default to the value used for the previous label"

I meant to say that solution #3 would give the described surprising results.
Comment 2 Ross Burton 2019-08-06 22:12:30 UTC
Reassigning to Marek, the u-boot maintainer.
Comment 3 Randy MacLeod 2019-08-08 14:44:33 UTC
Will, can you send a patch to the email list to fix this issue? Marek may be on vacation for a bit.
Comment 4 Richard Purdie 2019-08-08 14:53:12 UTC
The use of overrides in this way is perhaps unfortunate. What we try to do now is use a prefix with overrides to try and avoid this problem. Perhaps we could adopt some prefix here to avoid these problems?

Will: Would you be willing to send a patch for this?
Comment 5 Will Page 2019-08-08 15:16:17 UTC
(In reply to comment #4)
> The use of overrides in this way is perhaps unfortunate. What we try to do
> now is use a prefix with overrides to try and avoid this problem. Perhaps we
> could adopt some prefix here to avoid these problems?
> 
> Will: Would you be willing to send a patch for this?

Can you point me to an example of prefixed overrides, so that I'm clear on what you're suggesting and can do it consistently with other places in the project?
Comment 6 Will Page 2019-08-13 17:45:11 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > The use of overrides in this way is perhaps unfortunate. What we try to do
> > now is use a prefix with overrides to try and avoid this problem. Perhaps we
> > could adopt some prefix here to avoid these problems?
> > 
> > Will: Would you be willing to send a patch for this?
> 
> Can you point me to an example of prefixed overrides, so that I'm clear on
> what you're suggesting and can do it consistently with other places in the
> project?

I think I've got it.  I'm working on a solution that follows a similar route to what happens in devtools-source.bbclass.
Comment 7 Randy MacLeod 2019-08-13 19:56:47 UTC
Reset the target milestone. (2.8 is gone, it's now known as 3.0)
Comment 8 Randy MacLeod 2019-11-07 16:06:00 UTC
Will can you work on this or is it already fixed?
Comment 9 Will Page 2019-11-11 19:14:38 UTC
This was fixed in master with commit 42b44fdd9e63b8d39cf83baecec08328275558ff.

https://git.openembedded.org/openembedded-core/commit/meta/classes/uboot-extlinux-config.bbclass?id=42b44fdd9e63b8d39cf83baecec08328275558ff