[Zorg] Honor clean_obj in WebUI.

[Meinersbur]

Summary

Delete the build dir if either clean_obj or clean is true. Previously, build dir would not be cleaned if clean is false, even if clean_obj is true.

For reference, when which of the properties are set:

Normal build:

"clean": does not exist
"clean_obj": does not exist

Commit with CMakeLists.txt change:

"clean": does not exist
"clean_obj": [true, "change"]

"Force Build" -> "Clean source code and build directory" checked:

"clean": true
"clean_obj": false

"Force Build" -> "Clean build directory" checked:

"clean": false
"clean_obj": true

"Force Build" -> "Clean source code and build directory" and "Clean build directory" checked:

"clean": true
"clean_obj": true

[Meinersbur]

@gkistanova ping

[Meinersbur]

@gkistanova ping

[JonChesterfield]

The existing logic to delete the source directory might be there because the test programs write stuff into the source directory, so a clean build needs to blow that away.

Can we do a halfway-change here? In particular, if the downloaded repos are managed by git or similar, we could 'clean' the directories by git clean -fd instead of rm, which preserves the clean build behaviour and avoids redownloading the data.

[Meinersbur]

In D107193#2952208, @JonChesterfield wrote:

The existing logic to delete the source directory might be there because the test programs write stuff into the source directory, so a clean build needs to blow that away.

That's not a/the reason. Currently, zorg/LLVM's buildbot setup never deletes or cleans the the llvm-project.git checkout (unless done manually) and this has not been a problem so far. Buildbot runs git reset --hard has part of its normal checkout step.

This patch tries to apply the same logic to the llvm-test-suite.git checkout as it currently does to the llvm-project.git checkout.

Can we do a halfway-change here? In particular, if the downloaded repos are managed by git or similar, we could 'clean' the directories by git clean -fd instead of rm, which preserves the clean build behaviour and avoids redownloading the data.

git clean -xfd as part of the checkout process might be a good idea in case any files (particularly __pycache__). However, this is not what this patch is trying to fix.

[Meinersbur]

@gkistanova ping

[Meinersbur]

@gkistanova ping

[Meinersbur]

@gkistanova ping

[Meinersbur]

@gkistanova ping

[Meinersbur]

@gkistanova ping

[Meinersbur]

@gkistanova ping

[Meinersbur]

@gkistanova ping

[Meinersbur]

@gkistanova ping

[gkistanova]

Hello Michael,

Sorry for the late response.

The logic of the cleaning things should work like this:

1. If clean build property is set, the source code, the build, and the install directories are removed before corresponding steps.
2. If clean build property is NOT set, and if clean arg is set, or if clean_obj is set, then the build, and the install directories are removed before corresponding steps.

If a builder implements in-tree build, then it should implement "build tree removal" by invoking an appropriate git clean command for the case #2, and do nothing for the case #1, since the source code directory removal also removes the build directory.

In OpenMPBuilder.py UnifiedTreeBuilder would take care of this logic for the llvm-project part. So, you just need to implement the described logic for the testsuite_srcdir, sollvevv_srcdir and testsuite_builddir respectively.

I do not see this in this patch. What am I missing?

PollyBuilder.py does implement this logic for the llvm-project part as well. The only thing that needs a fix is the test suite. That part should support the cleaning for both - the test suite source code directory and the test suite build directory. Cleaning the test suite build directory seems missing in your patch.

If you think the naming cleanBuildRequestedByProperty and cleanBuildRequested is not so good and would like to propose better names, that's fine. Please make it a separate NFC patch for review.

UnifiedTreeBuider.getLLVMBuildFactoryAndPrepareForSourcecodeSteps ignores its cleanBuildRequested parameter and instead always uses

Only for removing the source code directory. It does use cleanBuildRequested to figure out if build and install directories should be removed. I described above the logic of cleaning. Please feel free to ask if you have questions.

[Meinersbur]

In D107193#3194088, @gkistanova wrote:

1. If clean build property is set, the source code, the build, and the install directories are removed before corresponding steps.
2. If clean build property is NOT set, and if clean arg is set, or if clean_obj is set, then the build, and the install directories are removed before corresponding steps.

I do not see this in this patch. What am I missing?

In addition to be "not set", the clean property can also be "false". Should this count as "set"? The Web UI has a feature to rebuild, where "clean" and "clean_obj" can be set to true or force by clicking the checkbox, but it can never be "not set". See the summary of this patch.

My interpretation is that "not set" is equivalent to "false". build/install dirs are cleared if wither "clean" or "clean_obj" is true. Source dirs/checkouts are cleared if "clean" is true. That is, "clean" encompasses "clean_obj". The test-suite checkout should be handled the same way as the llvm-project checkout.

UnifiedTreeBuider.getLLVMBuildFactoryAndPrepareForSourcecodeSteps ignores its cleanBuildRequested parameter and instead always uses

Only for removing the source code directory. It does use cleanBuildRequested to figure out if build and install directories should be removed. I described above the logic of cleaning.

From the source:

def getLLVMBuildFactoryAndPrepareForSourcecodeSteps(...
           cleanBuildRequested = None,
           ...):

    def cleanBuildRequestedByProperty(step):
        return step.build.getProperty("clean")

    if cleanBuildRequested is None:
        cleanBuildRequested = cleanBuildRequestedByProperty

    f = LLVMBuildFactory(...
            cleanBuildRequested=cleanBuildRequested,
            ...)

    f.addStep(steps.RemoveDirectory(name='clean-src-dir',
              dir=f.monorepo_dir,
              haltOnFailure=False,
              flunkOnFailure=False,
              doStepIf=cleanBuildRequestedByProperty,  # <-----
              ))

That is, the doStepIf parameter of the clean-src-dir is always passed cleanBuildRequestedByProperty, never what is passed by cleanBuildRequested. LLVMBuildFactory takes cleanBuildRequested as kwargs but only sets it as a object property and AFAIK, never uses it.

It is correct that most callers of getLLVMBuildFactoryAndPrepareForSourcecodeSteps also pass the cleanBuildRequested to other steps, such as addCmakeSteps. However, I was only referring to getLLVMBuildFactoryAndPrepareForSourcecodeSteps itself.

Please feel free to ask if you have questions.

How would you implement not deleting the llvm-test-suite git checkout at every run?

[gkistanova]

In addition to be "not set", the clean property can also be "false". Should this count as "set"?

Sorry for not being clear. With a flag “not set" I meant "anything that evaluates to false", and with "set" I meant "anything that evaluates to true". But you have figured that out.

The Web UI has a feature to rebuild, where "clean" and "clean_obj" can be set to true or force by clicking the checkbox

Yes, Web UI has its limitations. Web UI is not the only way we can specify build properties, though.

However, I was only referring to getLLVMBuildFactoryAndPrepareForSourcecodeSteps itself.

Yes, that makes sense. I just pointed for other readers that it passes the given cleanBuildRequested to the LLVMBuildFactory for later use on the steps related to build and install to handle the cleaning properly.

How would you implement not deleting the llvm-test-suite git checkout at every run?

You already have that in getPollyBuildFactory. And for getOpenMPCMakeBuildFactory you are after something similar to https://reviews.llvm.org/rZORG5ba5d2e80969. That should cover your immediate need, unless I'm missing something.

While you are at this, you may also want to add handling for cleanBuildRequested and conditionally remove testsuite_builddir in the both OpenMPBuilder.py and PollyBuilder.py.

I agree, the names cleanBuildRequested and cleanBuildRequestedByProperty are not so great. Not sure the cleanBuildRequested and cleanObjRequested are much better, though, since cleanBuildRequested changes semantic. Anyway, if you want, please feel free to propose NFC patch that renames everywhere. This would help keeping the names consistent. Or I'll do this when I'll have a chance.

[Meinersbur]

In D107193#3214206, @gkistanova wrote:

In addition to be "not set", the clean property can also be "false". Should this count as "set"?

Sorry for not being clear. With a flag “not set" I meant "anything that evaluates to false", and with "set" I meant "anything that evaluates to true". But you have figured that out.

This unfortunately does not correspond to the current source:

step.build.getProperty("clean", default=step.build.getProperty("clean_obj")

where the docstring of getProperty is:

Get the named property, returning the default if the property does not exist.

That is, if clean is set to False, the value of clean_obj does not matter, even if it is True and the build dir is not cleaned. This is what this patch intends to change.

The Web UI has a feature to rebuild, where "clean" and "clean_obj" can be set to true or force by clicking the checkbox

Yes, Web UI has its limitations. Web UI is not the only way we can specify build properties, though.

Sure, but I found it useful to tell my bots to use a fresh directory even if the next build does not include a CMakeLists.txt change in case there is some leftover. The alternative would be to login to the worker and rm -rf manually, which might interfere with a current build. I don't see a reason why to not make it work.

However, I was only referring to getLLVMBuildFactoryAndPrepareForSourcecodeSteps itself.

Yes, that makes sense. I just pointed for other readers that it passes the given cleanBuildRequested to the LLVMBuildFactory for later use on the steps related to build and install to handle the cleaning properly.

If it is intentional that getLLVMBuildFactoryAndPrepareForSourcecodeSteps ignores its cleanBuildRequested parameter, it should be documented. As it, it seems that callers should be able to control when to clean directories. In any case callers can also pass None for default behaviour.

How would you implement not deleting the llvm-test-suite git checkout at every run?

You already have that in getPollyBuildFactory. And for getOpenMPCMakeBuildFactory you are after something similar to https://reviews.llvm.org/rZORG5ba5d2e80969. That should cover your immediate need, unless I'm missing something.

Thank you. I think that should fix the most pressing problem of not clearing the test-suite source dir at every run.

While you are at this, you may also want to add handling for cleanBuildRequested and conditionally remove testsuite_builddir in the both OpenMPBuilder.py and PollyBuilder.py.

I agree, the names cleanBuildRequested and cleanBuildRequestedByProperty are not so great. Not sure the cleanBuildRequested and cleanObjRequested are much better, though, since cleanBuildRequested changes semantic. Anyway, if you want, please feel free to propose NFC patch that renames everywhere. This would help keeping the names consistent. Or I'll do this when I'll have a chance.

What names do you propose?

[Meinersbur]

• Rebase

[Meinersbur]

To clarify, in addition to renaming, I am proposing to change the semantics to match what you described in https://reviews.llvm.org/D107193#3194088. The new name "cleanObjRequested" is meant to reflect that. Renaming without this change of semantics IMHO does not make sense because the current cleanBuildRequestedByProperty does something else than described in https://reviews.llvm.org/D107193#3194088.

[gkistanova]

That is, if clean is set to False, the value of clean_obj does not matter, even if it is True and the build dir is not cleaned. This is what this patch intends to change.

Ah. I missed this point, sorry. Yes, you are right. There is a bug there.

Thanks for updating the patch!

It looks good, you just missed couple places with doStepIfs.

zorg/buildbot/builders/OpenMPBuilder.py

158

cleanBuildRequested here?

173

cleanBuildRequested here?

[Meinersbur]

• Some forgotten cleanBuildRequestedByProperty

[Meinersbur]

zorg/buildbot/builders/OpenMPBuilder.py

173

Yes. Thank you for noticing!

[gkistanova]

LGTM

zorg/buildbot/builders/OpenMPBuilder.py

def getOpenMPCMakeBuildFactory(
 
     llvm_srcdir = 'llvm.src'
     llvm_builddir = 'llvm.build'
     llvm_instdir = 'llvm.inst'
     testsuite_srcdir = "test-suite.src"
     testsuite_builddir = "test-suite.build"
     sollvevv_srcdir = "sollvevv.src"
 
-    cleanBuildRequested = lambda step: clean or step.build.getProperty("clean", default=step.build.getProperty("clean_obj"))
-    cleanBuildRequestedByProperty = lambda step: step.build.getProperty("clean")
+    # If true, clean everything, including source dirs
+    def cleanBuildRequested(step):
+        return step.build.getProperty("clean")
+    # If true, clean build products; implied if cleanBuildRequested is true
+    def cleanObjRequested(step):
+        return cleanBuildRequested(step) or clean or step.build.getProperty("clean_obj")
 
     if depends_on_projects is None:
         # Monorepo configuration requires llvm and clang to get cmake work.
         depends_on_projects = ['llvm', 'clang', 'openmp']
 
     f = UnifiedTreeBuilder.getLLVMBuildFactoryAndSourcecodeSteps(
             depends_on_projects=depends_on_projects,
             enable_runtimes=enable_runtimes,
             llvm_srcdir=llvm_srcdir,
             obj_dir=llvm_builddir,
             cleanBuildRequested=cleanBuildRequested,
             env=merged_env,
             **kwargs) # Pass through all the extra arguments.
 
-    f.addStep(
-        ShellCommand(
-            name            = 'clean',
-            command         = ['rm', '-rf', f.obj_dir],
-            warnOnFailure   = True,
-            description     = ['clean'],
-            doStepIf        = cleanBuildRequested,
-            workdir         = '.',
-            env             = merged_env))
+    f.addStep(steps.RemoveDirectory(name='clean',
+                           dir=f.obj_dir,
+                           warnOnFailure=True,
+                           doStepIf=cleanObjRequested))
 
     # Configure LLVM and OpenMP (and Clang, if requested).
     cmake_args = ['-DCMAKE_BUILD_TYPE=Release', '-DLLVM_ENABLE_ASSERTIONS=ON']
     if ompt:
         cmake_args += ['-DLIBOMP_OMPT_SUPPORT=ON']
     if test:
         lit_args = '-vv --show-unsupported --show-xfail -j %s' % jobs
         cmake_args += [WithProperties('-DLLVM_LIT_ARGS=%s' % lit_args)]
if install:
         libdir = "%(builddir)s/" + llvm_instdir + "/lib"
 
 
     if testsuite:
         f.addStep(steps.RemoveDirectory(name="Test-Suite: Clean Source",
                            dir=testsuite_srcdir,
                            haltOnFailure=False,
                            warnOnFailure=True,
-                           doStepIf=cleanBuildRequestedByProperty))
+                           doStepIf=cleanBuildRequested))
 
         f.addGetSourcecodeForProject(name="Test-Suite: Checkout",
             description="fetching",
             descriptionDone="fetch",
             descriptionSuffix="Test-Suite",
             project='test-suite',
             src_dir=testsuite_srcdir,
             alwaysUseLatest=True)
 
         if testsuite_sollvevv:
             f.addStep(steps.RemoveDirectory(name="SOLLVE V&V: Clean Source",
                            dir=sollvevv_srcdir,
                            haltOnFailure=False,
                            warnOnFailure=True,
-                           doStepIf=cleanBuildRequestedByProperty))
+                           doStepIf=cleanBuildRequested))
 
             f.addStep(steps.Git(name="SOLLVE V&V: Checkout",
                     description="fetching",
                     descriptionDone="fetch",
                     descriptionSuffix="SOLLVE V&V",
                     repourl='https://github.com/SOLLVE/sollve_vv.git',
                     workdir=sollvevv_srcdir,
                     alwaysUseLatest=True))

zorg/buildbot/builders/PollyBuilder.py

def getPollyBuildFactory(
     merged_env = {
                    'TERM' : 'dumb'     # Make sure Clang doesn't use color escape sequences.
                  }
     if env:
         merged_env.update(env)  # Overwrite pre-set items with the given ones, so user can set anything.
 
     depends_on_projects = ['llvm','clang','polly']
 
-    cleanBuildRequestedByProperty = lambda step: step.build.getProperty("clean", False)
-    cleanBuildRequested = lambda step: clean or step.build.getProperty("clean", default=step.build.getProperty("clean_obj"))
+    # If true, clean everything, including source dirs
+    def cleanBuildRequested(step):
+        return step.build.getProperty("clean")
+    # If true, clean build products; implied if cleanBuildRequested is true
+    def cleanObjRequested(step):
+        return cleanBuildRequested(step) or clean or step.build.getProperty("clean_obj")
 
     f = LLVMBuildFactory(
             depends_on_projects=depends_on_projects,
             llvm_srcdir=llvm_srcdir,
             obj_dir=llvm_objdir,
             install_dir=llvm_instdir,
             cleanBuildRequested=cleanBuildRequested,
             **kwargs) # Pass through all the extra arguments.
 
     f.addStep(steps.RemoveDirectory(name='clean-src-dir',
                            dir=f.monorepo_dir,
                            warnOnFailure=True,
-                           doStepIf=cleanBuildRequestedByProperty))
+                           doStepIf=cleanBuildRequested))
 
     # Get the source code.
     f.addGetSourcecodeSteps(**kwargs)
 
     # Clean build dir
     f.addStep(steps.RemoveDirectory(name='clean-build-dir',
                            dir=llvm_objdir,
                            warnOnFailure=True,
-                           doStepIf=cleanBuildRequested))
+                           doStepIf=cleanObjRequested))
 
     # Create configuration files with cmake
     cmakeCommand = ["cmake", "../%s/llvm" % llvm_srcdir,
                     "-DCMAKE_COLOR_MAKEFILE=OFF",
                     "-DPOLLY_TEST_DISABLE_BAR=ON",
                     "-DCMAKE_BUILD_TYPE=Release",
                     "-DLLVM_POLLY_LINK_INTO_TOOLS=ON",
                     "-DLLVM_ENABLE_PROJECTS=%s" % ";".join(f.depends_on_projects),
def getPollyBuildFactory(
     litexe = "%(builddir)s/" + llvm_objdir + "/bin/llvm-lit"
     sizeexe = "%(builddir)s/" + llvm_objdir + "/bin/llvm-size"
 
     # Clean install dir
     if install:
         f.addStep(steps.RemoveDirectory(name='clean-install-dir',
                                dir=llvm_instdir,
                                haltOnFailure=False,
-                               doStepIf=cleanBuildRequested))
+                               doStepIf=cleanObjRequested))
 
         f.addStep(ShellCommand(name="install",
                                command=install_cmd,
                                haltOnFailure=True,
                                description=["install"],
                                workdir=llvm_objdir,
                                env=merged_env))
 
else:
                                workdir=llvm_objdir,
                                env=merged_env))
 
     if testsuite:
         f.addStep(steps.RemoveDirectory(name='test-suite_clean-src-dir',
                            dir=testsuite_srcdir,
                            haltOnFailure=False,
                            warnOnFailure=True,
-                           doStepIf=cleanBuildRequestedByProperty))
+                           doStepIf=cleanBuildRequested))
 
         f.addGetSourcecodeForProject(
             project='test-suite',
             src_dir=testsuite_srcdir,
             alwaysUseLatest=True)
 
         f.addStep(steps.RemoveDirectory(name='test-suite_clean-build-dir',
                            dir=testsuite_builddir,