Page MenuHomePhabricator

[zorg] Add Polly builder for ARM target
ClosedPublic

Authored by pzheng on Jan 9 2017, 3:12 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

pzheng updated this revision to Diff 83711.Jan 9 2017, 3:12 PM
pzheng retitled this revision from to [zorg] Add Polly builder for ARM target.
pzheng updated this object.
pzheng added a subscriber: llvm-commits.
grosser edited edge metadata.Jan 9 2017, 7:47 PM

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)

pzheng updated this revision to Diff 83905.Jan 10 2017, 5:36 PM
pzheng marked 5 inline comments as done.Jan 10 2017, 5:44 PM

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.

pzheng added inline comments.Jan 10 2017, 5:48 PM
buildbot/osuosl/master/config/status.py
174 ↗(On Diff #83711)

We want to have someone monitor the builder, thus adding this extra stuff.

grosser accepted this revision.Jan 10 2017, 10:33 PM
grosser edited edge metadata.

Thank you!

This revision is now accepted and ready to land.Jan 10 2017, 10:33 PM
rengolin added inline comments.Jan 11 2017, 1:19 AM
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?

pzheng added inline comments.Jan 11 2017, 10:34 AM
zorg/buildbot/builders/PollyBuilder.py
100 ↗(On Diff #83905)

The install dir will be used by another builder. More patches coming soon.

This revision was automatically updated to reflect the committed changes.