buildman: Make -I the default

At present buildman defaults to running 'mrproper' on every thread before
it starts building commits for each board. This can add a delay of about 5
seconds to the start of the process, since the tools and other invariants
must be rebuilt.

In particular, a build without '-b', to build current source, runs much
slower without -I, since any existing build is removed, thus losing the
possibility of an incremental build.

Partly this behaviour was to avoid strange build-system problems caused by
running 'make defconfig' for one board and then one with a different
architecture. But these problems were fixed quite a while ago.

The -I option (which disabled mrproper) was introduced four years ago and
does not seem to cause any problems with builds.

So make -I the default and deprecate the option. To allow use of
'mrproper', add a new -m flag.

Signed-off-by: Simon Glass <sjg@chromium.org>

tools/buildman/README

@@ -958,12 +958,11 @@ will build commits in us-buildman that are not in upstream/master.
 Building Faster
 ===============
 
-By default, buildman executes 'make mrproper' prior to building the first
+By default, buildman doesn't execute 'make mrproper' prior to building the
-commit for each board. This causes everything to be built from scratch. If you
+first commit for each board. This reduces the amount of work 'make' does, and
-trust the build system's incremental build capabilities, you can pass the -I
+hence speeds up the build. To force use of 'make mrproper', use -the -m flag.
-flag to skip the 'make mproper' invocation, which will reduce the amount of
+This flag will slow down any buildman invocation, since it increases the amount
-work 'make' does, and hence speed up the build. This flag will speed up any
+of work done on any build.
-buildman invocation, since it reduces the amount of work done on any build.
 
 One possible application of buildman is as part of a continual edit, build,
 edit, build, ... cycle; repeatedly applying buildman to the same change or
@@ -994,7 +993,7 @@ Combining all of these options together yields the command-line shown below.
 This will provide the quickest possible feedback regarding the current content
 of the source tree, thus allowing rapid tested evolution of the code.
 
-    SOURCE_DATE_EPOCH=0 ./tools/buildman/buildman -I -P tegra
+    SOURCE_DATE_EPOCH=0 ./tools/buildman/buildman -P tegra
 
 
 Checking configuration

tools/buildman/builder.py

@@ -231,7 +231,7 @@ class Builder:
     def __init__(self, toolchains, base_dir, git_dir, num_threads, num_jobs,
                  gnu_make='make', checkout=True, show_unknown=True, step=1,
                  no_subdirs=False, full_path=False, verbose_build=False,
-                 incremental=False, per_board_out_dir=False,
+                 mrproper=False, per_board_out_dir=False,
                  config_only=False, squash_config_y=False,
                  warnings_as_errors=False, work_in_output=False):
         """Create a new Builder object
@@ -252,8 +252,7 @@ class Builder:
             full_path: Return the full path in CROSS_COMPILE and don't set
                 PATH
             verbose_build: Run build with V=1 and don't use 'make -s'
-            incremental: Always perform incremental builds; don't run make
+            mrproper: Always run 'make mrproper' when configuring
-                mrproper when configuring
             per_board_out_dir: Build in a separate persistent directory per
                 board rather than a thread-specific directory
             config_only: Only configure each build, don't build it
@@ -311,7 +310,7 @@ class Builder:
         self.queue = queue.Queue()
         self.out_queue = queue.Queue()
         for i in range(self.num_threads):
-            t = builderthread.BuilderThread(self, i, incremental,
+            t = builderthread.BuilderThread(self, i, mrproper,
                     per_board_out_dir)
             t.setDaemon(True)
             t.start()

tools/buildman/builderthread.py

@@ -90,12 +90,12 @@ class BuilderThread(threading.Thread):
         thread_num: Our thread number (0-n-1), used to decide on a
                 temporary directory
     """
-    def __init__(self, builder, thread_num, incremental, per_board_out_dir):
+    def __init__(self, builder, thread_num, mrproper, per_board_out_dir):
         """Set up a new builder thread"""
         threading.Thread.__init__(self)
         self.builder = builder
         self.thread_num = thread_num
-        self.incremental = incremental
+        self.mrproper = mrproper
         self.per_board_out_dir = per_board_out_dir
 
     def Make(self, commit, brd, stage, cwd, *args, **kwargs):
@@ -243,7 +243,7 @@ class BuilderThread(threading.Thread):
                 # If we need to reconfigure, do that now
                 if do_config:
                     config_out = ''
-                    if not self.incremental:
+                    if self.mrproper:
                         result = self.Make(commit, brd, 'mrproper', cwd,
                                 'mrproper', *args, env=env)
                         config_out += result.combined

tools/buildman/cmdline.py

@@ -55,8 +55,9 @@ def ParseArgs():
     parser.add_option('-i', '--in-tree', dest='in_tree',
           action='store_true', default=False,
           help='Build in the source tree instead of a separate directory')
+    # -I will be removed after April 2021
     parser.add_option('-I', '--incremental', action='store_true',
-          default=False, help='Do not run make mrproper (when reconfiguring)')
+          default=False, help='Deprecated, does nothing. See -m')
     parser.add_option('-j', '--jobs', dest='jobs', type='int',
           default=None, help='Number of jobs to run at once (passed to make)')
     parser.add_option('-k', '--keep-outputs', action='store_true',
@@ -69,6 +70,8 @@ def ParseArgs():
           default=False, help='Show a list of boards next to each error/warning')
     parser.add_option('--list-tool-chains', action='store_true', default=False,
           help='List available tool chains (use -v to see probing detail)')
+    parser.add_option('-m', '--mrproper', action='store_true',
+          default=False, help="Run 'make mrproper before reconfiguring")
     parser.add_option('-n', '--dry-run', action='store_true', dest='dry_run',
           default=False, help="Do a dry run (describe actions, but do nothing)")
     parser.add_option('-N', '--no-subdirs', action='store_true', dest='no_subdirs',

tools/buildman/control.py

@@ -172,6 +172,10 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None,
         print()
         return 0
 
+    if options.incremental:
+        print(col.Color(col.RED,
+                        'Warning: -I has been removed. See documentation'))
+
     # Work out what subset of the boards we are building
     if not boards:
         if not os.path.exists(options.output_dir):
@@ -309,7 +313,7 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None,
             show_unknown=options.show_unknown, step=options.step,
             no_subdirs=options.no_subdirs, full_path=options.full_path,
             verbose_build=options.verbose_build,
-            incremental=options.incremental,
+            mrproper=options.mrproper,
             per_board_out_dir=options.per_board_out_dir,
             config_only=options.config_only,
             squash_config_y=not options.preserve_config_y,

tools/buildman/func_test.py

@@ -476,15 +476,15 @@ class TestFunctional(unittest.TestCase):
         self._RunControl('-b', TEST_BRANCH, '-c2', '-o', self._output_dir)
         self.assertEqual(self._builder.count, 2 * len(boards))
         self.assertEqual(self._builder.fail, 0)
-        # Each board has a mrproper, config, and then one make per commit
+        # Each board has a config, and then one make per commit
-        self.assertEqual(self._make_calls, len(boards) * (2 + 2))
+        self.assertEqual(self._make_calls, len(boards) * (1 + 2))
 
     def testIncremental(self):
         """Test building a branch twice - the second time should do nothing"""
         self._RunControl('-b', TEST_BRANCH, '-o', self._output_dir)
 
         # Each board has a mrproper, config, and then one make per commit
-        self.assertEqual(self._make_calls, len(boards) * (self._commits + 2))
+        self.assertEqual(self._make_calls, len(boards) * (self._commits + 1))
         self._make_calls = 0
         self._RunControl('-b', TEST_BRANCH, '-o', self._output_dir, clean_dir=False)
         self.assertEqual(self._make_calls, 0)
@@ -496,14 +496,26 @@ class TestFunctional(unittest.TestCase):
         self._RunControl('-b', TEST_BRANCH, '-o', self._output_dir)
         self._make_calls = 0
         self._RunControl('-b', TEST_BRANCH, '-f', '-o', self._output_dir, clean_dir=False)
-        # Each board has a mrproper, config, and then one make per commit
+        # Each board has a config and one make per commit
-        self.assertEqual(self._make_calls, len(boards) * (self._commits + 2))
+        self.assertEqual(self._make_calls, len(boards) * (self._commits + 1))
 
     def testForceReconfigure(self):
         """The -f flag should force a rebuild"""
         self._RunControl('-b', TEST_BRANCH, '-C', '-o', self._output_dir)
-        # Each commit has a mrproper, config and make
+        # Each commit has a config and make
-        self.assertEqual(self._make_calls, len(boards) * self._commits * 3)
+        self.assertEqual(self._make_calls, len(boards) * self._commits * 2)
+
+    def testForceReconfigure(self):
+        """The -f flag should force a rebuild"""
+        self._RunControl('-b', TEST_BRANCH, '-C', '-o', self._output_dir)
+        # Each commit has a config and make
+        self.assertEqual(self._make_calls, len(boards) * self._commits * 2)
+
+    def testMrproper(self):
+        """The -f flag should force a rebuild"""
+        self._RunControl('-b', TEST_BRANCH, '-m', '-o', self._output_dir)
+        # Each board has a mkproper, config and then one make per commit
+        self.assertEqual(self._make_calls, len(boards) * (self._commits + 2))
 
     def testErrors(self):
         """Test handling of build errors"""
@@ -525,7 +537,7 @@ class TestFunctional(unittest.TestCase):
         self._RunControl('-b', TEST_BRANCH, '-o', self._output_dir, '-F', clean_dir=False)
         self.assertEqual(self._builder.count, self._total_builds)
         self.assertEqual(self._builder.fail, 0)
-        self.assertEqual(self._make_calls, 3)
+        self.assertEqual(self._make_calls, 2)
 
     def testBranchWithSlash(self):
         """Test building a branch with a '/' in the name"""