New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 854764 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

unibuild firmware packaging handles cros_ec flag incorrectly

Project Member Reported by jbrandmeyer@chromium.org, Jun 20 2018

Issue description

Seen on the scarlet-paladin CQ.  There were no changes picked up by this build that look like they directly affect scarlet, so this appears to be a bug on ToT/Cloud Storage.  It appears that the binary package contents were changed in such a way that the ebuild can no longer pick them up.

https://luci-milo.appspot.com/buildbot/chromeos/scarlet-paladin/4326


Log excerpts:
=== Start output for job chromeos-firmware-scarlet-0.0.1-r106 (0m8.2s) ===

...
chromeos-firmware-scarlet-0.0.1-r106: INFO:root:RunCommand: sh /build/scarlet/tmp/portage/chromeos-base/chromeos-firmware-scarlet-0.0.1-r106/work/chromeos-firmware-scarlet-0.0.1/chromeos-firmwareupdate --sb_repack /build/scarlet/tmp/portage/chromeos-base/chromeos-firmware-scarlet-0.0.1-r106/temp/tmpaSFQ68.pack_firmware-25319
chromeos-firmware-scarlet-0.0.1-r106: Package Content:
...
chromeos-firmware-scarlet-0.0.1-r106: 16809f21e7a566a8ef855bd75f24fba1 *./models/dru/ec.bin
...

chromeos-firmware-scarlet-0.0.1-r106: IOError: [Errno 2] No such file or directory: '/build/scarlet/firmware/scarlet/ec.bin'
 

Comment 1 by sjg@chromium.org, Jun 20 2018

Status: Started (was: Untriaged)
I don't know what is going on. Perhaps a config error. I will take a look.

Comment 2 by sjg@chromium.org, Jun 20 2018

It looks like scarlet does not define USE=cros_ec

Perhaps add this:

USE="${USE} cros_ec"

to:

src/overlays/overlay-scarlet/profiles/base/make.defaults

This should probably be done by the scarlet SIE

Comment 3 by sjg@chromium.org, Jun 20 2018

Cc: nsanders@chromium.org
Owner: philipchen@chromium.org
Status: Assigned (was: Started)
->Philip who I think is the right person
Cc: philipchen@chromium.org
Owner: jclinton@chromium.org
Looks like scarlet was surprise-unibuilded incorrectly without letting the SIE team know. Passing to the build team.


Owner: philipchen@chromium.org
Build team TL is vapier@ but I'm pretty sure this has nothing to do with them or unibuild. AFAIK, no one is actively converting boards. Please find a root cause.
Owner: vapier@chromium.org
Looks like scarlet was migrated by Charles before he left. As per previous discussion w/ Charles he owns build maintenance on migrated systems.
After switching to unibuild, why would chromeos-firmware-scarlet depend on locally-built firmware artifacts?

chromeos-firmware-scarlet-0.0.1-r106: IOError: [Errno 2] No such file or directory: '/build/scarlet/firmware/scarlet/ec.bin'

Actually, I've asked the same question before but didn't get answered:

https://b.corp.google.com/issues/80497318#comment5

I can probably resolve this by adding the USE flag to build chromeos-ec.
But please let me know why we need this dependency.
chromeos-firmware-${BOARD} should only pack the firmware binaries downloaded from BCS.
ec build targets get removed after awhile in TOT so the BCS based firmware packager must not rely on the presence of a locally built ec.
Look like the problem is in https://cs.corp.google.com/chromeos_public/src/third_party/chromiumos-overlay/eclass/cros-firmware.eclass?l=309 unibuild implementation, which does not handle cros_ec USE flag correctly
This issue will mostly be 'fixed' after this patch lands:
https://chromium-review.googlesource.com/c/chromiumos/overlays/board-overlays/+/1109378

But in the long run, we'll need a real fix as #9 described.
Cc: vapier@chromium.org sjg@chromium.org jclinton@chromium.org
Labels: -Pri-0 Pri-1
Summary: unibuild firmware packaging handles cros_ec flag incorrectly (was: Scarlet firwmare packaging error)
bumping priority down as the builder should be fixed with the above workaround.

vapier@ to fix or reassign build support for the root cause. 
+cc sjg@ as well as he wrote the code.

Comment 12 by sjg@chromium.org, Jun 21 2018

Owner: sjg@chromium.org
So I gather that the fix is to skip EC and PD from the local firmware build if USE=cros_ec is not set. I can do that.
I think that will do it, yes, but I'm not too familiar with that code. Thanks!
Project Member

Comment 14 by bugdroid1@chromium.org, Jun 21 2018

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

commit 7c2e9229eb139d218e8eab06d7ab81f0ee741c16
Author: Nick Sanders <nsanders@chromium.org>
Date: Thu Jun 21 08:05:51 2018

overlay-scarlet: add cros_ec USE flag.

scarlet was unibuilded incorrectly. This temporarily works around
an apparent bug in the unibuild implementation of chromeos-firmware
that requires a source built ec binary in order to package the one
from bcs.

BUG= chromium:854764 
TEST=./build_packages

Change-Id: I142042296be4ddd9e36fafe1af48ac59b7b38c30
Reviewed-on: https://chromium-review.googlesource.com/1109378
Commit-Ready: Philip Chen <philipchen@chromium.org>
Tested-by: Nick Sanders <nsanders@chromium.org>
Reviewed-by: Philip Chen <philipchen@chromium.org>

[modify] https://crrev.com/7c2e9229eb139d218e8eab06d7ab81f0ee741c16/overlay-scarlet/profiles/base/make.defaults

Cc: mojahsu@chromium.org grundler@chromium.org
 Issue 854884  has been merged into this issue.
Project Member

Comment 17 by bugdroid1@chromium.org, Jun 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/firmware/+/cab05a6fc1b2fa84169ae9e423884807c4001d85

commit cab05a6fc1b2fa84169ae9e423884807c4001d85
Author: Simon Glass <sjg@chromium.org>
Date: Fri Jun 22 02:56:19 2018

Allow building local firmware without EC and PD

At present we require the EC and PD to be included in a local firmware
build. But at the eclass level this is actually controlled by a USE flag
(cros_ec). In that case we don't want to include these binaries.

Update the logic and comments to support this possibility. There is no
test coverage for this case.

BUG= chromium:854764 
TEST=with a change to cros-firmware.eclass:
emerge-scarlet --unmerge chromeos-ec
FEATURES=test USE=-cros_ec emerge-scarlet chromeos-firmware-scarlet
See that this happily builds a local firmware image now and tests pass

Also:
FEATURES=test USE=-bootimage emerge-scarlet chromeos-firmware-scarlet

See that it does not build a local image, tests still pass

Change-Id: Ie59ca201906dbd947a0fb6db1380675d8a357fa4
Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1110025
Reviewed-by: Philip Chen <philipchen@chromium.org>
Reviewed-by: Nick Sanders <nsanders@chromium.org>

[modify] https://crrev.com/cab05a6fc1b2fa84169ae9e423884807c4001d85/pack_firmware.py

Project Member

Comment 18 by bugdroid1@chromium.org, Jun 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/b3af86b8283221d72ce08926b2eb4fc90845ee26

commit b3af86b8283221d72ce08926b2eb4fc90845ee26
Author: Simon Glass <sjg@chromium.org>
Date: Fri Jun 22 21:13:21 2018

cros-firmware: Support local firmware package without EC

At present with unibuild we require that the EC and PD iamges be present
in order to build a firmware image based on locally built binaries (as
opposed to 'official' ones downloaded from BCS).

Prior to unibuild the cros_ec USE flag was used to control this. Add this
same behaviour for unibuild.

This allows chromeos-firmware-scarlet to build. The problem was not
noticed for a long time since EC binaries were still present from before
it was converted to unibuild.

Note that we provide both EC and PD filenames to pack_firmware.py but it
checks the master config to see which ones are needed. For example, it
will only include the PD binary if the config has a 'pd-image' property.
If there is no 'pd-image' then the PD filename provided will be ignored,
and it is fine to provide is anyway.

BUG= chromium:854764 
TEST=with go/croscl/1110025:
emerge-scarlet --unmerge chromeos-ec
FEATURES=test USE=-cros_ec emerge-scarlet chromeos-firmware-scarlet
See that this happily builds a local firmware image now and tests pass

Change-Id: Ideab9531c91b7c23f00d857d38ffdc09f870c2eb
Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1110029
Reviewed-by: Nick Sanders <nsanders@chromium.org>

[modify] https://crrev.com/b3af86b8283221d72ce08926b2eb4fc90845ee26/eclass/cros-firmware.eclass

Comment 19 by sjg@chromium.org, Jun 25 2018

Status: Fixed (was: Assigned)
Project Member

Comment 20 by bugdroid1@chromium.org, Jun 26 2018

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

commit 30593cb1185ba36c9f6a85cdbebdb67002188910
Author: Simon Glass <sjg@chromium.org>
Date: Tue Jun 26 19:52:00 2018

Revert "overlay-scarlet: add cros_ec USE flag."

This reverts commit 7c2e9229eb139d218e8eab06d7ab81f0ee741c16.

Reason for revert: Now that  crbug.com/854764  is fixed, we should not need this?

Original change's description:
> overlay-scarlet: add cros_ec USE flag.
>
> scarlet was unibuilded incorrectly. This temporarily works around
> an apparent bug in the unibuild implementation of chromeos-firmware
> that requires a source built ec binary in order to package the one
> from bcs.
>
> BUG= chromium:854764 
> TEST=./build_packages
>
> Change-Id: I142042296be4ddd9e36fafe1af48ac59b7b38c30
> Reviewed-on: https://chromium-review.googlesource.com/1109378
> Commit-Ready: Philip Chen <philipchen@chromium.org>
> Tested-by: Nick Sanders <nsanders@chromium.org>
> Reviewed-by: Philip Chen <philipchen@chromium.org>

Bug:  chromium:854764 
Change-Id: I9f8a99d42864c86577bc5047a6a6bb4df5d90504
Reviewed-on: https://chromium-review.googlesource.com/1113778
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Nick Sanders <nsanders@chromium.org>

[modify] https://crrev.com/30593cb1185ba36c9f6a85cdbebdb67002188910/overlay-scarlet/profiles/base/make.defaults

thanks Simon!

Sign in to add a comment