Collapse buildrequests regardless of clean(_obj) property

[Meinersbur]

With this PR, the clean and clean_obj properties are ignored when considering whether to collapse two buildrequests. The collapsed request will have that property if either of the original requests has. clean, respectively clean_obj, not being set never guaranteed pre-populated build guranteed anyway.

Background:

• The clean property is set in the Force Build dialog: "Clean source code and build directory". It is never set by LLVMPoller.
• The clean_obj property is set in the Force Build dialog: "Clean build directory", or when LLVMPoller adds a buildrequest for a new llvm-project commit that changes a CMake file or contains the string "Require[...]clean build" in the commit message.
• Neither is automatically set on builders even with clean option. Instead, those builders enable the cleaning step independent of the clean/clean_obj properties inherited from the buildrequest.

Two buildrequests were only collapsed when their properties are equal, including the clean_obj property. Since the value of the property changes dependending on whether a CMakeLists.txt file is touched, a typical sequence of buildrequests after collapsing becomes

1. buildrequest
2. buildrequest with clean_obj=true
3. buildrequest
4. buildrequest with clean_obj=true
5. buildrequest
6. buildrequest with clean_obj=true
7. buildrequest
8. ...

Hence, slow builders with collapseRequests enabled in practice it still does two builds per CMakeLists.txt change. This may lead to high response latency during times of high commit volume or the slowest builders not being able to keep up at all.

[DavidSpickett]

Currently we only ignore differences in the clean_obj setting, and only if the builder would do a clean build anyway.

What you're doing is:

• Extending that to clean (which seems to do the same thing as clean_obj?)
• Making the assertion that this can be done even on builders that do not clean build by default, because the final collapsed request will have clean and/or clean_obj set if one of the 2 original requests did.

Did I understand correctly?

[DavidSpickett]

My original PR for reference - #266

[Meinersbur]

What this does:

1. When considering collapsing merges, ignore clean and clean_obj properties.
2. The request are merged, set clean/clean_obj on the merged buildrequest, if at least one of the buildrequest has that property set.

This is different from the mechnism that #266 introduced in that it is able to set the clean/clean_obj property itself, whereas #266 only handles clean_obj and requires that another component (in this case: builder.config.factory.clean) already applies that same effect as clean_obj would. Hence, it only works if builder.config.factory.clean == True (i.e. incremental builds disabled). This PR also works for incremental builders.

This PR also ignores the buildrequest clean property (that #266 still requires to match) which conceptually makes no difference here, but I don't have a use case. I still implemented it following the rule of least astonishment.

I think that #266 does not work as intended. The clean property is basically never set, nor do we want it to be set as it means that git clone has to re-populate the llvm.src directory. This means the condition if getattr(builder.config.factory, "clean", False): never applies.

Since it checks builder.config.factory (not the buildrequest properties), it actually might work for Factories that support it. The interpretation of clean is different than the buildrequest clean here. AnnotatedBuilder, which I am working with, uses the term clobber instead, which might be why I didn't notice it actally collapsing.

which seems to do the same thing as clean_obj?

• Dialog description of clean: "Clean source code and build directory"
• Dialog description of clean_obj: "Clean build directory"

Please read the background section in the summary.

[DavidSpickett]

I understand this now. A lot of my confusion was that my previous change definitely did do something for some of the builders, but I do now see how this PR expands on that.

Looks fine to me, but since this interacts with the database, Galina should be the approver.

And for them, the inspiration for the new setter method is here - https://github.com/buildbot/buildbot/blob/0aee5e842fcbb8b98e90fce01ea871e675445ce8/master/buildbot/db/builds.py#L320

[Meinersbur]

Note that as part of #648, I am trying to encourage to make as many decisions as possible, including whether to do an incremental build, into the build script. That is, an incremental builder can be changed to be non-incremental without a master restart, and vice versa. The master itself wouldn't have the information anymore.