New issue
Advanced search Search tips

Issue 643323 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 14
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Feature



Sign in to add a comment

Better customization of Chrome command lines for Chrome-OS-based boards

Project Member Reported by vapier@chromium.org, Sep 1 2016

Issue description

we've wanted people to not check for board values when building the chrome command line, but rather push for feature flags.  that's fine.

sometimes though, you really have board-specific junk, and feature flags don't quite cut it.  the code reflects this:
libchromeos-ui/chromeos/ui/chromium_command_builder.cc:      const bool is_atom = IsBoard("x86-alex") || IsBoard("x86-alex_he") ||
libchromeos-ui/chromeos/ui/chromium_command_builder.cc:          IsBoard("x86-mario") || IsBoard("x86-zgb") || IsBoard("x86-zgb_he");
libchromeos-ui/chromeos/ui/chromium_command_builder.cc:  if (IsBoard("link") || IsBoard("link_freon")) {
login_manager/chrome_setup.cc:  if (builder->IsBoard("kevin") || builder->IsBoard("kevin-tpm2")) {
login_manager/chrome_setup.cc:  if (builder->IsBoard("veyron_minnie"))
login_manager/chrome_setup.cc:  if (builder->IsBoard("veyron_rialto")) {
login_manager/chrome_setup.cc:  if (builder->IsBoard("veyron_mickey"))

maybe we should capitulate slightly here and add support for an /etc/chrome_board.conf file that boards can install.  then we can purge the IsBoard func entirely from the source tree.
 

Comment 1 by derat@chromium.org, Sep 1 2016

Most of those aren't board-specific, though:

- is_atom seems like it could (should?) be replaced by a USE flag, if those switches are even needed anymore.
- link/link_freon seems like it's working around touchpad issues (so this _is_ board-specific). I don't think that link_freon exists anymore.
- kevin/kevin-tpm2's switches are going to be controlled by a USE flag after the hardware ships.
- veyron_rialto's switches are going to be controlled by a "rialto" USE flag (https://chromium-review.googlesource.com/c/375358/, currently in CQ).
- veyron_minnie's --enable-hardware-overlays switch seems like it should be either replaced by a USE flag or removed in the future.

I still have the same objections to chrome*.conf files living in overlays that I mentioned earlier:

1. A config-file-based approach falls down as soon as we want to set something for a base board and then modify it for a variant.
2. Instead of keeping flags needed by atom devices in one place, they'll be duplicated across five overlays.
3. A config-file-based approach doesn't work for flags whose values are computed at runtime (which doesn't seem to be the case for any of the current IsBoard-guarded flags, but is for plenty of other flags).

I'm happy to help clean up the existing stuff, but I'd still prefer to not see Chrome switches defined in overlays.
is_atom is really just a proxy for "the CPU in this board is slow".  we could change it to something like "disable_low_latency_audio", but those boards are EOL and dying hopefully soonish.

link_freon is dead and can be cleaned.

renaming the rialto board check to USE=rialto is semantics.  the rialto board is the only one who wants that specific set of USE flags ... it's not like it's controlling one or two.

however, we can add as many/few USE flags as we want, but it still won't help -- the starting point for this was that we have people using CrOS to do their own builds and they don't have a way of customizing things cleanly.  the recommendation is to do it dirty and have the overlay's scripts/board_specific_setup.sh:board_finalize_base_image() function hack up the "${root_fs_dir}/etc/chrome_dev.conf" file.  at least one board in the tree is already doing this.

Comment 3 by derat@chromium.org, Sep 1 2016

Summary: Better customization of Chrome command lines for Chrome-OS-based boards (was: login_manager: support /etc/chrome_board.conf)
Re rialto, their intent (as I understand it) is to remove this customization and use kiosk mode -- see https://docs.google.com/document/d/1oj2vZA3smu2yCXAluHnDdqap2791MTW-Sedv9l6DCL8/edit#heading=h.8f5nq2mi1oza.

Re checked-in chrome_dev.conf files: yay. My favorite copy is the one that adds switches immediately below the comment saying not to check in modified versions of the file.

If this is a frequent use case, I'm fine with finding a cleaner way to support it. I want to do it from the perspective of identifying developers' needs and coming up with a good solution, though. chrome_dev.conf is basically "this thing here looks sort of like what I need so I'll use it."

chrome_dev.conf (or renamed files using the same parser) just hack the standard Chrome command line, which can change at any time. Are developers using a stock Chrome binary or their own customized versions? If it's the stock one, why are they okay with landing changes to it upstream but not with editing chrome_setup.cc?

Should I start an internal doc to enumerate the existing use cases?
i grok that rialto wants to drop the code entirely and use the standard enterprise/kiosk modes.  but i think that is a good example of every once in a while, a board needs an escape hatch.  i don't think inserting that logic directly into platform2/ makes sense, regardless of it being a board or USE flag.  we allowed it because we have closeish ties with the rialto people, but it's not the situation we want to be in forever.

we're talking about external people using existing flags -- they won't generally be landing any code into CrOS.  i don't think we want to force them to modify the platform2 code just to pass in their own flags.  yes, Chromium changes, and that might cause them problems eventually, but that's not our problem :).

Comment 5 by derat@chromium.org, Sep 2 2016

I've convinced myself that I'm okay with this approach as long as it's only used for external, not-checked-in overlays. I don't want our internal command lines to end up scattered across many different repositories. I'm tempted to enforce that with a presubmit check, if I can think of a way to do so... :-P
we can make an autotest to enforce it

let's consider the rialto case though ... i'm not convinced it's better to have the list hardcoded in the common source

Comment 7 by derat@chromium.org, Sep 2 2016

You mean something rialto-like that continues using custom flags over the long term, right? I guess I'm still hung up on the suspicion that it's rare to need to set a bunch of custom flags but not touch the Chrome code that implements those flags. I suspect that a more common scenario is that the flags are being implemented in Chrome specifically for these boards.
i mean even in the short to mid term for rialto.  if a Google board wants to spin up quickly, having to throw hacks into common code is ... not ideal.  we're still cleaning up hacks from tegra2 boards (i just posted a CL today for it).
Project Member

Comment 9 by bugdroid1@chromium.org, Sep 2 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/08b3532995d999c14e49a871b49daadb52ef4ffe

commit 08b3532995d999c14e49a871b49daadb52ef4ffe
Author: Daniel Erat <derat@chromium.org>
Date: Thu Sep 01 20:25:48 2016

Add pineview USE flag to x86-{alex,mario,zgb}.

Introduce a pineview USE flag for systems with early Atom
CPUs, intended to prevent the need for a hardcoded board
list in libchromeos-ui.

BUG= chromium:643323 
TEST=none

Change-Id: I1caa238ff7517cc0cf9d8badea8efa32bf9629c7
Reviewed-on: https://chromium-review.googlesource.com/380255
Commit-Ready: Dan Erat (ooo friday) <derat@chromium.org>
Tested-by: Dan Erat (ooo friday) <derat@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/08b3532995d999c14e49a871b49daadb52ef4ffe/overlay-x86-mario/make.conf
[modify] https://crrev.com/08b3532995d999c14e49a871b49daadb52ef4ffe/overlay-x86-alex/make.conf
[modify] https://crrev.com/08b3532995d999c14e49a871b49daadb52ef4ffe/overlay-x86-zgb/make.conf

Project Member

Comment 10 by sheriffbot@chromium.org, Sep 4 2017

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Status: Assigned (was: Untriaged)
Status: Fixed (was: Assigned)
i think we've fixed this now

Sign in to add a comment