This is an archive of the discontinued LLVM Phabricator instance.

make AnnotatedBuilder use LLVMBuildFactory and depends_on_projects
ClosedPublic

Authored by inglorion on Jun 9 2017, 1:48 PM.

Details

Summary

This makes AnnotatedBuilder derive from LLVMBuildFactory
instead of the standard buildbot factory. This means it will have
the same properties and methods as the other builders - in particular,
depends_on_projects, which is used for scheduling.

Diff Detail

Repository
rL LLVM

Event Timeline

inglorion created this revision.Jun 9 2017, 1:48 PM
rnk added a comment.Jun 12 2017, 9:49 AM

Do you think we should use LLVMBuildFactory.addSVNSteps to drive the checkout process, or is it better to keep that as part of the annotator script? Keeping it in the script lets us add more repos without doing buildmaster restarts, which is nice, but if we do that, then the scheduling logic gets out of sync.

What do you mean by "the scheduling logic gets out of sync"?

rnk added a comment.Jun 12 2017, 5:12 PM

What do you mean by "the scheduling logic gets out of sync"?

Suppose ThinLTO develops a dependency on libprofile in compiler-rt. If we update the annotator script to check out compiler-rt without updating the depends_on_projects list on the buildmaster, then builds will not be scheduled after compiler-rt changes. By using depends_on_projects, we're moving the knowledge of what source to check out back into the build master config. If adding a new subproject dependency requires a buildmaster restart, maybe we should go back to using the master side SVN checkout steps.

I see. Yes, it's unfortunate that this essentially repeats the list of projects in multiple places. On the other hand, I like having as much as possible in the part that can be modified without requiring a buildmaster reload, which would speak for leaving the checkout code in the builder. This is also what the sanitizer builders do. They trigger on changes to any of llvm, cfe, compiler-rt, libcxx, libcxxabi, libunwind, and lld, which we could also do here. It's basically triggering on everything, which is safe. Theoretically, it might increase resource usage a bit, but the ThinLTO builder as-is is basically always building, so the number of builds triggered and the amount of work the worker does should be the same. Would you like me to make that change?

rnk accepted this revision.Jun 13 2017, 3:52 PM

I see. Yes, it's unfortunate that this essentially repeats the list of projects in multiple places. On the other hand, I like having as much as possible in the part that can be modified without requiring a buildmaster reload, which would speak for leaving the checkout code in the builder. This is also what the sanitizer builders do. They trigger on changes to any of llvm, cfe, compiler-rt, libcxx, libcxxabi, libunwind, and lld, which we could also do here. It's basically triggering on everything, which is safe. Theoretically, it might increase resource usage a bit, but the ThinLTO builder as-is is basically always building, so the number of builds triggered and the amount of work the worker does should be the same. Would you like me to make that change?

Not really. Let's leave it as is, I guess.

This revision is now accepted and ready to land.Jun 13 2017, 3:52 PM
This revision was automatically updated to reflect the committed changes.