[Lldb-commits] [PATCH] D69555: [zorg] Fix LLDBCmakeBuildFactory

Thu Oct 31 01:16:40 PDT 2019

labath marked an inline comment as done.
labath added a comment.

In D69555#1727882 <https://reviews.llvm.org/D69555#1727882>, @stella.stamenova wrote:

> Sadly, I think this broke the windows bot now (assuming it got applied to staging, otherwise I have to figure out what did):
>
> http://lab.llvm.org:8014/builders/lldb-x64-windows-ninja/builds/46/steps/cmake-configure/logs/stdio

I am afraid that was due to 425eeb1b <https://reviews.llvm.org/rZORG425eeb1bf21b7f507905f17970f667263d630a81> (or its combination with this patch, if you will).  I am afraid we have too many quotes now :(

In an separate email, @gkistanova wrote:

> In D69555 <https://reviews.llvm.org/D69555> the problem of quotations around -DLLVM_ENABLE_PROJECTS remains.
>
> > "-DLLVM_ENABLE_PROJECTS=" + ";".join(f.depends_on_projects),

Actually, that's not true. :( The way that this patch resolved the quoting problem is by passing the command as an array. You'll notice that originally the command was being passed as a string, while now it's an array of strings. One does not need to add extra quotes when passing commands as arrays, as the array already provides the information about argument boundaries. Adding the extra quotes is not necessary and it seems to break stuff in some situations. The linux bot does not seem to be affected by this (which actually surprises me a lot), but the windows bot gets utterly confused by that.

I think that just removing the extra quotes should make everything happy.

================
Comment at: zorg/buildbot/builders/LLDBBuilder.py:110

-    cmake_cmd = [
-        "cmake", "-G", "Ninja", os.path.join(os.pardir, f.monorepo_dir, "llvm"),
+    path = os.path.join(os.pardir, f.monorepo_dir, "llvm")
+    cmake_options = [
----------------
gkistanova wrote:
> This would build a path per notations on master. Which could be different than what a builder expects. Think of master running on Windows, for example.
> 
> Better use a "/" path separator as these days it seems supported by all platforms we care about.
Thanks. I didn't know about the `LLVMBuildFactory.pathRelativeToBuild` stuff.

Repository:
  rZORG LLVM Github Zorg

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

https://reviews.llvm.org/D69555