[PATCH] D107193: [Zorg] Don't delete test-suite source directory every time.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

In D107193#3194088 <https://reviews.llvm.org/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?


Repository:
  rZORG LLVM Github Zorg

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107193/new/

https://reviews.llvm.org/D107193