New issue
Advanced search Search tips

Issue 842590 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

add unittest for overlay include order, use flags, rather than unsetting use flags

Project Member Reported by nsanders@chromium.org, May 14 2018

Issue description

Currently "target-chrome-os" overrides a bunch of use flags which are not required to be disabled.

Historically, it seems the intention was to "test" that the use flags were set in baseboard rather than overlay. However they frequently cause unexpected build output and unnecessary debugging, since test "failure" simply reports success, but causes defective build output. This has wasted many hours of debug time across a number of projects.

If a test is desired, an actual CQ or presubit test is needed, with an obvious failure message. USE flags shouldn't be unset silently for "test" purposes.   

See:
https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/profiles/targets/chromeos/make.defaults
* We should remove all unsets from this file.
03:30PM
Component: ChromeOS > Packages
 

Comment 1 by vapier@chromium.org, May 14 2018

Components: Infra>Client>ChromeOS>Build
Owner: vapier@chromium.org
Status: WontFix (was: Unconfirmed)
as i mentioned multiple times at this point [1] [2] [3], this statement is incorrect:
> Currently "target-chrome-os" overrides a bunch of use flags which are not required to be disabled.

ebuilds do not have this power.  you've shown no data to prove otherwise.  the referenced CL [1] attributed behavior to the wrong thing as i explained there as well too in the comments.

as for your other request:
> profiles/targets/chromeos/make.defaults: We should remove all unsets from this file.

that is impossible.  profiles stack, and we turn on/off flags for the CrOS profile that get enabled/disabled elsewhere.  this is how profiles have always worked, and is desired behavior.

wrt USE=-eclog in that profile, as i mentioned in the CL [1], you're free to drop that if you want.

so at this point, the fundamental request in this bug is either not a thing (invalid), or is undesirable because it'd break things.  which means you're left with fixing the overlays long term [4] and backporting one of the simple workarounds for existing branches [5] [6].  neither of which are the responsibility of the build team, and since you already have a bug open for fixing meowth derivatives, this bug itself has no value, so i'm punting it.

[1] https://chromium-review.googlesource.com/1053594
[2] http://b/78569903
[3] http://b/79598191
[4] https://chromium-review.googlesource.com/1053560
[5] https://chromium-review.googlesource.com/1053594
[6] https://chromium-review.googlesource.com/1053592
As in the initial bug description, the request is to drop USE=-eclog in make.defaults associated with this target. Sounds like we agree.

Comment 3 by vapier@chromium.org, May 14 2018

nowhere in this bug or in b/79598191 did you mention "eclog".  you only ever talked in large generalities, none of which we wanted.

Bob already posted https://chromium-review.googlesource.com/1053594 to drop -eclog from the make.defaults, but he kept trying to make unnecessary ebuild changes at the same time.

also, i think i've been pretty clear both in the CLs and in the bugs: dropping -eclog from make.defaults is a workaround, pure and simple.  the meowth overlays are broken and must be fixed.  treating CL:1053594 as the "correct" fix is ignoring the fundamental breakage the boards continue to have.
The generalized point of this bug is that setting USE flags in one overlay vs another is not "broken", but rather didn't conform to your style preferences.

If you want to enforce coding style, adding silent overrides in profiles/targets/chromeos/make.defaults is not the correct way to do it.

Comment 5 by vapier@chromium.org, May 15 2018

if you don't understand how the CrOS system profiles come together, and aren't familiar with the history (and who actually wrote these things), then don't deflect behind claims that i'm enforcing arbitrary style preferences.  it has nothing to do with that, and all you're doing is pointing fingers and wasting everyone's time.

in fact, i've never once set eclog variable or told anyone where to set them.  in fact, when that line was added to the profile 2 years ago, i commented at that time "the make.defaults line isn't really needed, but it also doesn't hurt".  but the author opted to go that route and i didn't tell them to do it one way or the other.

on the other hand, this issue does have to do with the right preference/stacking order so that we have a shared baseline architecture, a shared baseline OS (linux), a shared baseline distro (CrOS), a shared chipset, a shared baseboard, and finally the leaf overlays.  it avoids a ton of duplication of code and so people, when they want to update something at say the distro level, they don't have to copy & paste the same change to well over 100 files.

however, because the meowth overlays are written wrong, they directly declare that they want the chromeos settings to have a higher precedence.  the authors of those overlays made a mistake, and none of it has to do with "style".

as for "being silent", i'm not really sure what you reasonably expect here.  you want every flag/variable that gets overridden in every single file to generate a log warning on every emerge command ?  that's ridiculous.

so please stop these pointless blame games.
Cc: shapiroc@chromium.org
Status: Assigned (was: WontFix)
Summary: add unittest for overlay include order, use flags, rather than unsetting use flags (was: target-chromium-os incorrectly unsets use flags)
Mike, please chat with me directly if you don't understand what this bug is about. 

My understanding is that you have a preference for overlay stacking order and where use flags are set. My request is that you write an explicit unittest for this in the chromeos pre-CQ or somewhere appropriate. This will keep the overlays clean and ordered the way you like (and I generally agree with you on this) and easily convey this message to anyone working on the code. I've updated the bug title to hopefully be clearer.

Comment 7 by vapier@chromium.org, May 21 2018

Status: WontFix (was: Assigned)
this bug is not salvageable.

CrOS has settled on a design with profile stacks. much of it predates me, while the rest has come from ddocs reviewed by the CrOS team. so while it's true "I have a preference", it's one backed with technical reasons, existing art, and what the project has settled on.

this bug wasn't filed requesting better tooling, it was to break from all established routes to support a technically unsound board setup.

you can star issue 845071 and issue 845072 for tooling improvements.

Sign in to add a comment