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

Issue 642503 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Sep 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Run `make buildall` in chromeos-ec src_compile()

Project Member Reported by nxia@chromium.org, Aug 30 2016

Issue description

Yes, we definitely want to have such coverage. Can you paste the 2 bad CLs here so I can see how they were pre-cq tested?
Cc: aaboagye@chromium.org
Labels: -Pri-3 Pri-1
compile-only should be running "make -j buildall" as part of test for the chromeos-ec package. Is this not the case?
I don't know, but in the meantime while this is diagnosed we should restore the prior ec testing behavior.
Actually, it looks like it just builds for that board and not buildall. My bad, we can land the revert while we find a way to do this properly.
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/12b958dd8ad1e8d75308f924a0794448439d9b31

commit 12b958dd8ad1e8d75308f924a0794448439d9b31
Author: Aviv Keshet <akeshet@chromium.org>
Date: Tue Aug 30 22:05:55 2016

Revert "COMMIT-QUEUE.ini: Change builders to compile-only."

This reverts commit 654271b86c971d7ae32b3f8d798bd1c2c7171f4e.

Apparently ec changes can and do break unit tests on the cq.

BUG= chromium:642503 
TEST=None

Change-Id: I88cebf492a4c4a86b75a27ba823a03ffb705a2bb
Reviewed-on: https://chromium-review.googlesource.com/378415
Tested-by: Aviv Keshet <akeshet@chromium.org>
Reviewed-by: Aseda Aboagye <aaboagye@chromium.org>

[modify] https://crrev.com/12b958dd8ad1e8d75308f924a0794448439d9b31/COMMIT-QUEUE.ini

Cc: wfrichar@chromium.org
Owner: aaboagye@chromium.org
Status: Assigned (was: Untriaged)
I'll check to see if there's a builder config that will run the unittests or make one if necessary.
What we probably should do is have one ebuild that builds all the ToT images for all BOARDs supported in the EC firmware repo. Just running "make buildall" does that, and runs the unit tests too. Then we'd only need to do that once on a trybot and CQ.

The firmware shellball found in chromiumos_image.bin gets its firmware blobs from BCS. They're only put up there and marked valid after they've run through FAFT and other tests.

Any EC firmware blobs that are being built as part of a specific target disk image aren't being used (which was Aseda's point), so there's no need to be  doing it on every builder.




One other thing that I was thinking about was just having the src_compile() function just call "make buildall", which might be simpler.
We should chat. I had a brief email exchange with Bernie about this the other week.

Alright, sounds good.
Summary: Run `make buildall` in chromeos-ec src_compile() (was: ec unittest errors)
After discussing with wfrichar@ last week, long term we'd like to make a separate package that just builds all the EC images for all ToT boards. However in the meantime, we can still gain some benefit by just adding `make buildall` to the compile stage and use the compile-only-pre-cq builders again.
Status: Started (was: Assigned)
Filed  bug 644290  for the new package.
Project Member

Comment 16 by bugdroid1@chromium.org, Sep 8 2016

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

commit d33a2d6abef3a73756416d4a87aaaa30ddafae98
Author: Aseda Aboagye <aaboagye@google.com>
Date: Tue Sep 06 17:11:48 2016

chromeos-ec: Remove extra make command.

`make buildall` will also build the runtests target, so it's redundant
here.

BUG= chromium:642503 
BRANCH=None
TEST=emerge chromeos-ec

Change-Id: Id48fe28f4573bf52d22299d8d6de3ec83f37a461
Signed-off-by: Aseda Aboagye <aaboagye@google.com>
Reviewed-on: https://chromium-review.googlesource.com/381323
Commit-Ready: Aseda Aboagye <aaboagye@chromium.org>
Tested-by: Aseda Aboagye <aaboagye@chromium.org>
Reviewed-by: Bill Richardson <wfrichar@chromium.org>

[modify] https://crrev.com/d33a2d6abef3a73756416d4a87aaaa30ddafae98/chromeos-base/chromeos-ec/chromeos-ec-9999.ebuild

For posterity, we decided against adding `make buildall` to the compile stage and left it in the test stage. However, we are changing the pre-cq builders to no-vmtest builders. That way we'll still run the UnitTest stage, but not the VMTest or HWTest.
Project Member

Comment 18 by bugdroid1@chromium.org, Sep 8 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/64d6f5781becdf43b8de29a242277bc98b3b4b1e

commit 64d6f5781becdf43b8de29a242277bc98b3b4b1e
Author: Aseda Aboagye <aaboagye@google.com>
Date: Tue Sep 06 17:06:03 2016

COMMIT-QUEUE.ini: Change builders to no-vmtest-pre-cq.

(Re-attempt of https://chromium-review.googlesource.com/#/c/372325/)
Really, we only care if the chromeos-ec package unit tests fail and not
if the VMTest or ImageTest stages fail.  Those test stages don't
actually test aganist our EC changes anways, so it's kind of a waste of
time to run them.  Besides, that's what FAFT is for.

BUG= chromium:642503 
BRANCH=None
TEST=cbuildbot --remote chell-no-vmtest-pre-cq

Change-Id: I1b4b7fc68a9f8a943f6f5ef3d8b169264c95359e
Signed-off-by: Aseda Aboagye <aaboagye@google.com>
Reviewed-on: https://chromium-review.googlesource.com/381106
Commit-Ready: Aseda Aboagye <aaboagye@chromium.org>
Tested-by: Aseda Aboagye <aaboagye@chromium.org>
Reviewed-by: Bernie Thompson <bhthompson@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/64d6f5781becdf43b8de29a242277bc98b3b4b1e/COMMIT-QUEUE.ini

So, I don't think the change in c#18 is working correctly???

It seems that commit landed at 15:36 PDT. Here's an EC change that was uploaded after that (https://chromium-review.googlesource.com/#/c/382662/). However, the pre-cq builders selected are mixed pre-cq & compile-only...

What's going on here? Why weren't the no-vmtest-pre-cq builders chosen?
Wow, I think I figured it out. I had a mistake in the config, but someone with fresher eyes had to point it out to me. I uploaded another CL to fix this. https://chromium-review.googlesource.com/#/c/383878/

Why didn't cbuildbot complain??
Status: Verified (was: Started)

Sign in to add a comment