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

Wed Jan 5 06:09:01 PST 2022

Meinersbur added a comment.

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

Repository:
  rZORG LLVM Github Zorg

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

https://reviews.llvm.org/D107193