replacing `rm -rf` with RemoveDirectory step in ClangBuilders

[kuhnel]

Summary

I'm experiencing issues with deleting files on Windows builder, e.g. http://lab.llvm.org:8014/#/builders/27/builds/1200
The root cause is most likely a race condition when using the Unix rm -rfcommand. (Details in the video). BuildBot has an integrated RemoveDirectory step which calls rmdir which should not have these issues.

I also added a .pylint.rc file, so that I could use PyLint to check the modified code. Otherwise it would not parse the zorg.* packages.

[kuhnel]

added empty last line

[amccarth]

LGTM.

I look forward to just switching everything to RemoveDirectory.

[amccarth]

LGTM. (I must have slipped on the Action drop-down.)

[amccarth]

I think this patch is a good idea, but I'm having second thoughts about whether this will solve the problem.

Some details from the example build linked from the patch description:

rm: reading directory `stage1/include/llvm': Permission denied
rm: cannot remove directory `stage1/include': Directory not empty

The "Permission denied" message surprises me. If this were just the usual directory tree race condition, I'd only expect "Directory not empty" messages. Instead, it's as though there is an actual permission problem, which means something doesn't get deleted, and _that_'s why the parent directory is not empty.

I don't know how much to trust those messages. Is there actually a permission problem _reading_ the directory? Could there be some file with the wrong ACL or being held open by another thread?

I'll keep my fingers crossed that RemoveDirectory will resolve this, but I won't be surprised if the problem remains.

[kuhnel]

I can't really reproduce the issue outside of buildbot. Deleting the files locally always works.

There is nothing else running on the machine, that could keep an open file handle except some leftovers from a previous build/test.

[kuhnel]

removed backwards compatibility, directly replacing all clean actions

[amccarth]

I'm glad @gkistanova asked for this simplification. The more cautious approach added a lot of probably unnecessary noise.

[kuhnel]

Not pursuing work on the windows buildbot any more

.pylintrc

+# add root folder to Pylint PYTHONPATH, so builders can be checked with zorg.*
+# packages
+[MASTER]
+init-hook='import sys; sys.path.append(".")'
+
+[FORMAT]
+max-line-length=120

zorg/buildbot/builders/ClangBuilder.py

 import copy
 from datetime import datetime
 
 from buildbot.process.properties import WithProperties, Property
 from buildbot.steps.shell import ShellCommand, SetProperty
 from buildbot.steps.shell import WarningCountingShellCommand
+from buildbot.steps.worker import RemoveDirectory
 
 import zorg.buildbot.builders.Util as builders_util
 
 from zorg.buildbot.commands.LitTestCommand import LitTestCommand
 from zorg.buildbot.conditions.FileConditions import FileDoesNotExist
 from zorg.buildbot.commands.CmakeCommand import CmakeCommand
 from zorg.buildbot.process.properties import InterpolateToPosixPath
 from zorg.buildbot.process.factory import LLVMBuildFactory
if vs and vs != "manual":
         f.addStep(SetProperty(
             command=builders_util.getVisualStudioEnvironment(vs, vs_target_arch),
             extract_fn=builders_util.extractVSEnvironment))
         assert not env, "Can't have custom builder env vars with VS"
         env = Property('vs_env')
 
 
     ############# CLEANING
-    f.addStep(ShellCommand(name='clean stage 1',
-                           command=['rm','-rf',stage1_build],
+    # Use the buildbot build-in step "RemoveDirectory".
+    # http://docs.buildbot.net/current/manual/configuration/buildsteps.html#removedirectory
+    f.addStep(RemoveDirectory(name='clean stage 1',
+                              dir=stage1_build,
                               warnOnFailure=True,
                               haltOnFailure=False,
                               flunkOnFailure=False,
                               description='cleaning stage 1',
                               descriptionDone='clean',
                               workdir='.',
                               doStepIf=clean_build_requested))
 
 
     ############# STAGE 1
     CmakeCommand.applyRequiredOptions(extra_cmake_args, [
         ('-DLLVM_ENABLE_PROJECTS=', ";".join(f.depends_on_projects)),
         ])
     rel_src_dir = LLVMBuildFactory.pathRelativeTo(f.llvm_srcdir, stage1_build)
 
if test and testStage1:
                                  command=ninja_check_cmd,
                                  haltOnFailure=haltOnStage1Check,
                                  description=["checking stage 1"],
                                  descriptionDone=["stage 1 checked"],
                                  workdir=stage1_build,
                                  env=env))
 
     if useTwoStage or runTestSuite or stage1_upload_directory:
-        f.addStep(ShellCommand(name='clean stage 1 install',
-                               command=['rm','-rf',stage1_install],
-                               warnOnFailure=True,
-                               haltOnFailure=False,
-                               flunkOnFailure=False,
-                               description='cleaning stage 1 install',
-                               descriptionDone='clean',
-                               workdir='.'))
+        f.addStep(RemoveDirectory(name='clean stage 1 install',
+                            dir=stage1_install,
         f.addStep(ShellCommand(name='install stage 1',
                                command=ninja_install_cmd,
                                description='ninja install',
                                workdir=stage1_build,
                                env=env))
 
     if stage1_upload_directory:
         addGCSUploadSteps(f, 'stage 1', stage1_install, stage1_upload_directory,
def _getClangCMakeBuildFactory(
 
 
     ############# STAGE 2
     if useTwoStage:
         # We always cleanly build the stage 2. If the compiler has been
         # changed on the stage 1, we cannot trust any of the intermediate file
         # from the old compiler. And if the stage 1 compiler is the same, we
         # should not build in the first place.
-        f.addStep(ShellCommand(name='clean stage 2',
-                               command=['rm','-rf',stage2_build],
+        f.addStep(RemoveDirectory(name='clean stage 2',
+                                  dir=stage2_build,
                                   warnOnFailure=True,
                                   description='cleaning stage 2',
                                   descriptionDone='clean',
                                   workdir='.'))
 
+
         # Absolute paths to just built compilers.
         # Note: Backslash path separators do not work well with cmake and ninja.
         # Forward slash path separator works on Windows as well.
         stage1_cc = InterpolateToPosixPath(
                         "-DCMAKE_C_COMPILER=%(builddir)s/{}/bin/{}".format(
                             stage1_install,
                             cc))
         stage1_cxx = InterpolateToPosixPath(
if useTwoStage:
                                      env=env))
 
     ############# TEST SUITE
     ## Test-Suite (stage 2 if built, stage 1 otherwise)
     if runTestSuite:
         compiler_path = stage1_install
         if useTwoStage:
             compiler_path=stage2_install
-            f.addStep(ShellCommand(name='clean stage 2 install',
-                                   command=['rm','-rf',stage2_install],
+            f.addStep(RemoveDirectory(name='clean stage 2 install',
+                                      dir=stage2_install,
                                       warnOnFailure=True,
                                       description='cleaning stage 2 install',
                                       descriptionDone='clean',
                                       workdir='.'))
+
             f.addStep(ShellCommand(name='install stage 2',
                                    command=ninja_install_cmd,
                                    description='ninja install 2',
                                    workdir=stage2_build,
                                    env=env))
 
         # Get generated python, lnt
         python = WithProperties('%(builddir)s/test/sandbox/bin/python')
if runTestSuite:
             if testerName and not use_runtest_testsuite:
                 test_suite_cmd.extend(['--no-machdep-info', testerName])
         # CC and CXX are needed as env for build-tools
         test_suite_env = copy.deepcopy(env)
         test_suite_env['CC'] = cc
         test_suite_env['CXX'] = cxx
 
         # Steps to prepare, build and run LNT
-        f.addStep(ShellCommand(name='clean sandbox',
-                               command=['rm', '-rf', 'sandbox'],
+        f.addStep(RemoveDirectory(name='clean sandbox',
+                                  dir='sandbox',
                                   haltOnFailure=True,
                                   description='removing sandbox directory',
                                   workdir='test',
                                   env=env))
+
         f.addStep(ShellCommand(name='recreate sandbox',
                                command=['virtualenv', 'sandbox'],
                                haltOnFailure=True,
                                description='recreating sandbox',
                                workdir='test',
                                env=env))
         f.addStep(ShellCommand(name='setup lit',
                                command=[python, lnt_setup, 'develop'],