[PATCH] D87085: Add flang out of tree buildbot

[Galina via Phabricator via llvm-commits]

gkistanova added a comment.

Thanks for changing, Diana!

Almost there. Please see my inline comments.



================
Comment at: zorg/buildbot/builders/FlangBuilder.py:16
+    # Make a local copy of the llvm configure args, as we are going to modify that.
+    if llvm_extra_configure_args:
+        llvm_cmake_args = llvm_extra_configure_args[:]
----------------
`UnifiedTreeBuilder` would do this for you. You can just pass the `llvm_extra_configure_args` down to `getCmakeWithNinjaBuildFactory` as is.


================
Comment at: zorg/buildbot/builders/FlangBuilder.py:44
+
+    obj_dir = "build_flang"
+    src_dir = "%s/flang" % f.monorepo_dir
----------------
Just a cosmetic.
How about renaming this to something like `flang_obj_dir` and `flang_src_dir` respectively?
Otherwise it might look a bit confusing with `f.obj_dir` etc.


================
Comment at: zorg/buildbot/builders/FlangBuilder.py:52
+        # We actually need the paths to be relative to the source directory,
+        # otherwise find_package can't locate the config files.
+        ('-DLLVM_DIR:PATH=', LLVMBuildFactory.pathRelativeToBuild(llvm_dir, src_dir)),
----------------
The naming of `pathRelativeToBuild` method is not great. I refactored it to be `pathRelativeTo` instead. 

Could you rebase the patch, please? 
Otherwise it looks confusing, when the comment says we want paths relative to the source and then there are `pathRelativeToBuild` calls.




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

https://reviews.llvm.org/D87085