This is an archive of the discontinued LLVM Phabricator instance.

[zorg] Add libomp builders with OMPT
ClosedPublic

Authored by Hahnfeld on Apr 8 2016, 12:11 AM.

Details

Summary

Dmitri Gribenko said that it is ok to "add more (reasonably-sized) jobs to [his] buildbot" (http://lists.llvm.org/pipermail/openmp-dev/2016-March/001169.html)

Diff Detail

Event Timeline

Hahnfeld updated this revision to Diff 52996.Apr 8 2016, 12:11 AM
Hahnfeld retitled this revision from to [zorg] Add libomp builders with OMPT.
Hahnfeld updated this object.
Hahnfeld added a reviewer: gkistanova.
Hahnfeld added a project: Restricted Project.
Hahnfeld added subscribers: gribozavr, jlpeyton.
gkistanova edited edge metadata.Apr 13 2016, 3:15 PM

The patch looks fine.
Except one strange thing with the category.

buildbot/osuosl/master/config/builders.py
822

The category gets overwritten to 'openmp' in get_builders().
Why setting it here explicitly?

Hahnfeld added inline comments.Apr 14 2016, 4:09 AM
buildbot/osuosl/master/config/builders.py
822

Looks like I did some copy&paste from the other configs. Will update the patch and remove the category from all related buildes

Hahnfeld updated this revision to Diff 54035.Apr 17 2016, 11:11 PM
Hahnfeld edited edge metadata.

Remove category because it is set in get_builders anyway

gkistanova added inline comments.Apr 18 2016, 11:18 AM
buildbot/osuosl/master/config/builders.py
830

The env value looks specific to a particular buildslave rather than the builder. Do you expect each of the slaves be setup exactly the same way, including the user account name and such?

This is not a blocker for this patch, though. Just something that caught my eye.

zorg/buildbot/builders/Libiomp5Builder.py
93

Could you use the NinjaCommand from zorg.buildbot.commands.NinjaCommand instead, please?
Something like this would do:

f.addStep(NinjaCommand(name="build_llgo",
                       targets=["FileCheck"],
                       haltOnFailure=True,
                       description=["make ompt test utils"],
                       workdir=llvm_builddir,
                       env=merged_env))
98

You are missing ) here.

Hahnfeld updated this revision to Diff 54164.Apr 18 2016, 11:34 PM

Add missing )

Hahnfeld marked 3 inline comments as done.Apr 18 2016, 11:37 PM
Hahnfeld added inline comments.
zorg/buildbot/builders/Libiomp5Builder.py
93

Can this be done in a separate patch? This should then be adjusted in the whole file...

gkistanova accepted this revision.Apr 19 2016, 6:56 PM
gkistanova edited edge metadata.

LGTM, assuming the patch with NinjaCommand will follow shortly.

This revision is now accepted and ready to land.Apr 19 2016, 6:56 PM
This revision was automatically updated to reflect the committed changes.