Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Very nice!
Some small questions from my side. What is different in the getPollyARMBuildFactory compared to getPollyBuildFactory? In the optimal case, we just have one build factory that is parametrized to the specific builder, no? Maybe some of the changes could be moved to the getPollyBuildFactory that all Polly builders can benefit. We could e.g. make the 'make-command' just a parameter, which is set to 'ninja' for the ARM builder. Also, while at it, I would not call the build command 'Build Polly', but rather 'Build or Build+LLVM'. The reason is that this will come up in the error messages when a build fails. In many cases such failures are due to errors in LLVM, but if the name implies this is Polly-only people might skip those mails by accident.
Some comments on the implementation, but I share Tobias's concerns. Why not re-use the existing Poly builder?
buildbot/osuosl/master/config/status.py | ||
---|---|---|
174 ↗ | (On Diff #83711) | You don't need to add this to new builders. |
zorg/buildbot/builders/PollyBuilder.py | ||
177 ↗ | (On Diff #83711) | Paths should be configuration parameters (see the ARM buildbots) |
212 ↗ | (On Diff #83711) | Targets and triples should be configuration parameters (see the ARM buildbots) |
228 ↗ | (On Diff #83711) | Number of jobs should be a configuration parameter (see the ARM buildbots) |
243 ↗ | (On Diff #83711) | Number of jobs should be a configuration parameter (see the ARM buildbots) |
250 ↗ | (On Diff #83711) | Number of jobs should be a configuration parameter (see the ARM buildbots) |
Thanks for reviewing the patch!
I have refactored getPollyBuildFactory to take a few additional arguments and got rid of getPollyARMBuildFactory so that the builder for ARM also uses getPollyBuildFactory. Also addressed a few other comments from grosser and rengolin.
buildbot/osuosl/master/config/status.py | ||
---|---|---|
174 ↗ | (On Diff #83711) | We want to have someone monitor the builder, thus adding this extra stuff. |
buildbot/osuosl/master/config/status.py | ||
---|---|---|
174 ↗ | (On Diff #83711) | This should really pick up from the bot owner's email, not this hard-coded address. But that's not for this patch. |
zorg/buildbot/builders/PollyBuilder.py | ||
100 ↗ | (On Diff #83905) | Why do you need the install dir if you're not using it in the buildbot? |
zorg/buildbot/builders/PollyBuilder.py | ||
---|---|---|
100 ↗ | (On Diff #83905) | The install dir will be used by another builder. More patches coming soon. |