In general, we solve these things not by changing other flags automatically, but by having unittests that discover and notify you if what you are doing won't work. Adding more tests of this form to chromeos_config is really easy to do.

This is mostly to make sure the user is aware of consequences to their actions, instead of having unexpected side effects.

Ping. The beaglebone canary is still red because of the underlying
problem.

I prepared this test, but there are currently 2090 build configs that fail it.

Probably, because most of those build configs simply ignore the problematic build config values.

> I prepared this test, but there are currently 2090 build configs that fail it.

Ai ya.  Do we know which/how many boards (as opposed to configs) are
impacted?  Also, I think this unit test should mostly only be applicable
to canary/release builders.  Do we have some other class of builder that's
failing?

Yes, there are many classes of builder showing up as broken, most of them tryjob related. I also believe that every board is affected, for some types of builder.

I suspect the 'correct' fix is to turn off all behaviors in all build configs, and only turn them on when needed, then have tests in the builders that error out if we have a behavior enabled and don't use it. That way the flags tell us something useful without having to know if the relevant builder class/stages pay attention to them.

That is, of course, not going to happen since it would be a massive, and massively disruptive development effort.

https://chromium-review.googlesource.com/#/c/481228/

There's a small bug in the CL.  This:
      if config.hwqual or config.upload_hw_test_artifacts:

Should be this:
      if config.hwqual and config.upload_hw_test_artifacts:

Also, the code to count the number of failures doesn't do that.
It counts all of the builders that should have a test image
according to the rules, whether or not there's a test image.

There are actually only 92 build configs that fail the test;
90 of them are firmware builders.  The other two are
beaglebone-release, and beaglebone_servo-release.

Fixing the firmware configs to pass the test is trivial.
However, I don't (yet) know if the patch below is actually
the right thing to do:

diff --git a/cbuildbot/chromeos_config.py b/cbuildbot/chromeos_config.py
index a2b8a125..d4781978 100644
--- a/cbuildbot/chromeos_config.py
+++ b/cbuildbot/chromeos_config.py
@@ -1512,6 +1512,7 @@ def GeneralTemplates(site_config, ge_build_config):
       'firmware_base',
       site_config.templates.no_vmtest_builder,
       images=[],
+      hwqual=False,
       factory_toolkit=False,
       packages=['virtual/chromeos-firmware', 'chromeos-base/autotest-all'],
       usepkg_build_packages=True,

I ran a sample trybot against a ToT firmware builder:
    https://uberchromegw.corp.google.com/i/chromiumos.tryserver/builders/depthcharge_firmware/builds/0

True to expectations, the builder doesn't produce a test
image.  But apparently, it doesn't try to run ArchiveHWQual
(which is good, because the step would fail otherwise).

So, as best I can tell, the patch in c#9 is the right answer.

I've uploaded a fix to the unit test, but I have to ask:
The ArchiveHWQual() method already has a check that seems
to say "if my prerequisites are met and I'm configured to
run, do the work."  So, it seems like this patch would also
solve the problem as originally described:
diff --git a/cbuildbot/stages/artifact_stages.py b/cbuildbot/stages/artifact_stages.py
index b427cd5e..675003c2 100644
--- a/cbuildbot/stages/artifact_stages.py
+++ b/cbuildbot/stages/artifact_stages.py
@@ -248,7 +248,8 @@ class ArchiveStage(generic_stages.BoardSpecificBuilderStage,
       # TODO(petermayo): This logic needs to be exported from the BuildTargets
       # stage rather than copied/re-evaluated here.
       # TODO(mtennant): Make this autotest_built concept into a run param.
-      autotest_built = (self._run.options.tests and
+      autotest_built = ('test' in config.images and
+                        self._run.options.tests and
                         config['upload_hw_test_artifacts'])
 
       if config['hwqual'] and autotest_built:


That would tend to obviate the unit test _and_ the change to
the beaglebone builders.

I tend to TRY and avoid that type of logic because it makes it really hard to look at the configuration and figure out what the builder will actually do.

We have a LOT of that type of logic today, but I really prefer unittests that ensure the configuration is sane.

> We have a LOT of that type of logic today, but I really
> prefer unittests that ensure the configuration is sane.

Unit test fixes have already been uploaded, if that's the preferred
solution.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/d1b3814ec35d6914ac6346102872c9b32e994961

commit d1b3814ec35d6914ac6346102872c9b32e994961
Author: Don Garrett <dgarrett@google.com>
Date: Wed Apr 26 23:19:49 2017

chromeos_config_unittest: Require test image for hwqual.

The ArchiveHWQual step requires a test image.  Add a unit test that
disallows enabling 'hwqual' unless a test image is being built.

This change also fixes the firmware builder configs to pass the new
unit test.

BUG= chromium:710079 
TEST=Ran test.

Change-Id: I1c937c9cafb7c6638727c39b8d5aa7d37a5c1c71
Reviewed-on: https://chromium-review.googlesource.com/481228
Commit-Ready: Richard Barnette <jrbarnette@chromium.org>
Tested-by: Richard Barnette <jrbarnette@chromium.org>
Reviewed-by: Don Garrett <dgarrett@chromium.org>

[modify] https://crrev.com/d1b3814ec35d6914ac6346102872c9b32e994961/cbuildbot/config_dump.json
[modify] https://crrev.com/d1b3814ec35d6914ac6346102872c9b32e994961/cbuildbot/chromeos_config_unittest.py
[modify] https://crrev.com/d1b3814ec35d6914ac6346102872c9b32e994961/cbuildbot/chromeos_config.py