Page MenuHomePhabricator

[Zorg] Change make to ninja instead for Clang RHEL buildbot
Needs RevisionPublic

Authored by Conanap on Tue, Apr 13, 6:03 PM.

Details

Summary

Changing it to use ninja instead as it builds and runs with ninja.

Diff Detail

Event Timeline

Conanap requested review of this revision.Tue, Apr 13, 6:03 PM
Conanap created this revision.
Conanap updated this revision to Diff 337320.Tue, Apr 13, 7:57 PM
Conanap retitled this revision from [Zorg] Add quotes around Unix Makefiles to prevent cmake from assuming it's 2 args to [Zorg] Change make to ninja instead for Clang RHEL buildbot.
Conanap edited the summary of this revision. (Show Details)
Conanap added a reviewer: jsji.

Using ninja instead

stefanp added inline comments.Wed, Apr 14, 3:50 AM
zorg/buildbot/builders/ClangBuilder.py
498

It looks like you are modifying a step in ClangCMakeBuildFactory to use ninja instead of make. My worry is that this factory is used in a number of bots (see config/builders.py) not only PowerPC bots. It this going to cause problems on other platforms? Other bots?

Conanap added inline comments.Wed, Apr 14, 7:06 AM
zorg/buildbot/builders/ClangBuilder.py
498

Hey Stefan, this part of the patch (cmake_test_suite) was added just for the RHEL buildbot; you can see the other fabricator patch here (committed on Monday): https://reviews.llvm.org/D99097

saghir accepted this revision.Wed, Apr 14, 1:47 PM

This looks good to me, approving to unblock the failing bot.

This revision is now accepted and ready to land.Wed, Apr 14, 1:47 PM
stefanp accepted this revision.Wed, Apr 14, 1:51 PM

Thank you for the explanation!
LGTM.

This revision was landed with ongoing or failed builds.Wed, Apr 14, 1:53 PM
This revision was automatically updated to reflect the committed changes.
gkistanova reopened this revision.Wed, Apr 21, 11:49 PM

This patch has broken zorg. I reverted the both this one and D99097 as they seems closely related, and D100431 fixes D99097.

This revision is now accepted and ready to land.Wed, Apr 21, 11:49 PM
gkistanova requested changes to this revision.Thu, Apr 22, 12:04 AM

Sorry for reverting your patches, but I do not see a quick and easy way to fix the problems it has introduced.

Besides, with your last change to use ninja to run tests I don't think you need all these changes after all. It looks like you can use UnifiedTreeBuilder as is and it will do what you want. If not, we can discuss what is missing,

Though, for the sake of completeness of this review, please see my comments inline.

zorg/buildbot/builders/ClangBuilder.py
493

You cannot concatenate a string with WithProperties.
Trying to do this would give you a builtins.TypeError: must be str, not WithProperties error.

494

The same is here. No string and WithProperties concatenations.

556

lnt_setup is not defined if cmake_test_suite is True.

This revision now requires changes to proceed.Thu, Apr 22, 12:04 AM