[PATCH] D78955: [zorg] Add polly test-suite builder.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur marked an inline comment as done.
Meinersbur added inline comments.


================
Comment at: zorg/buildbot/builders/PollyBuilder.py:65
+    if testsuite:
+        # Get the builddir for use with WithProperties
+        f.addStep(SetProperty(name="get_builddir",
----------------
gkistanova wrote:
> You do not need a step to get the builddir. This is `workdir` property, just use that. For example,
> ```
> WithProperties("%(workdir)s/" + llvm_instdir + "/bin/clang++")
> ```
> would give you a fully qualified path to the installed clang++.
I copied the `get_builddir` step from ClangBuilder. Thanks for the hint.

It is/was wrapped into WithProperties when the step was added.


================
Comment at: zorg/buildbot/builders/PollyBuilder.py:114
 
+    clangexe   = os.path.join(llvm_objdir, 'bin', 'clang')
+    clangxxexe = os.path.join(llvm_objdir, 'bin', 'clang++')
----------------
gkistanova wrote:
> This code will be running on the master. The path on master has nothing to do with the path on the bot which will be actually building. Here and in thew rest of similar places, please use forward slashes here as it would work everywhere.
`llvm_objdir` is set to "llvm.obj" at the beginning of this function, i.e. this is a path relative to the workdir on slave, as it was used already by all the other steps.  I changed it to string concatenation as suggested. 

Personally, I think `os.path.join` more idiomatic as it e.g. also works on Windows and if any argument is an absolute path.


================
Comment at: zorg/buildbot/builders/PollyBuilder.py:181
+                                    WithProperties("-DTEST_SUITE_LIT=%(builddir)s/" + litexe),
+                                    WithProperties("-DTEST_SUITE_LIT_FLAGS=-v;-o;report.json"),
+                                    WithProperties("-DTEST_SUITE_LLVM_SIZE=%(builddir)s/" + sizeexe)
----------------
gkistanova wrote:
> Adding -vv lit flag might help troubleshooting test failures.
`-vv` only adds additional output when `execute_external` is used, which seems only true for `ABI-Testsuite`.


Repository:
  rZORG LLVM Github Zorg

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78955/new/

https://reviews.llvm.org/D78955