<div dir="ltr">Chris,<br><br>I am still not a fan of LLVM_TOOL_CLANG_*. It's external for me.<div>I suppose generic naming rule is not a matter.</div><div><br></div><div>I describe an alternate idea. Could you consider?</div><div><br></div><div>1) Don't generate LLVM_TOOL_*_SOURCE_DIR. Assume in-tree tools are not relocatable.</div><div><br></div><div>2) If corresponding source directory of each tool isn't present, let in-tree stuff do nothing.</div><div><br></div><div>3) external stuff may be suppressed if corresponding in-tree directory were there. (optional)</div><div><br></div><div>I have a suggestion around LLVM_TOOL_*.</div><div>Dozen of auto-generated LLVM_TOOL_*_BUILD might be nasty.</div><div>How about LLVM_TOOLS_TO_BUILD=llc;lli;llvm-as.... ?</div><div><br></div><div>...Takumi</div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, Jul 10, 2015 at 2:37 AM Chris Bieneman <<a href="mailto:beanz@apple.com">beanz@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Takumi,<div><br></div><div>I was looking at revising my patches to not rename the variables, and I realized it is actually very nasty to not rename this. Let me describe some of the rationale behind this cleanup.</div><div><br></div><div>Today we have a lot of duplication and confusion in our CMake code. At the highest level we have add_llvm_external_project and add_llvm_tool_subdirectory. Those two functions do basically the same thing, but in different ways. They both call the CMake add_subdirectory, they both have to append values to the implicit ignore list. add_llvm_tool_subdirectory lacks a way to disable in-tree tools, and so we have ignore_llvm_tool_subdirectory to add a tool to the implicit ignore list without adding the subdirectory, and we have logic in the tools/CMakeLists.txt to ignore tools.</div><div><br></div><div>The important thing to understand is that we already need and rely on having the ability to ignore both in-tree and out-of-tree tools, but we accomplish them in different ways.</div><div><br></div><div>The surface goal of this code is to simplify tools/CMakeLists.txt and make it so that adding tools (in-tree or out-of-tree) don’t require changes. To accomplish that goal I needed to eliminate the differences between add_llvm_external_project and add_llvm_tool_subdirectory so that the implicit discovery mechanism can be used as the default method for all subdirectories that don’t need special handling (see: Polly).</div><div><br></div><div>I chose to eliminate the differences by extending add_llvm_external_project to be the “one true way” to make this work.</div><div><br></div><div>With my patches the following code in tools/CMakeLists.txt:</div><div><br></div><div><div><font face="Menlo">if( LLVM_USE_INTEL_JITEVENTS )</font></div><div><font face="Menlo">  add_llvm_tool_subdirectory(llvm-jitlistener)</font></div><div><font face="Menlo">else()</font></div><div><font face="Menlo">  ignore_llvm_tool_subdirectory(llvm-jitlistener)</font></div><div><font face="Menlo">endif( LLVM_USE_INTEL_JITEVENTS )</font></div></div><div><br></div><div>Simplifies to:</div><div><br></div><div><div><font face="Menlo">if(NOT LLVM_USE_INTEL_JITEVENTS )</font></div><div><font face="Menlo">  set(LLVM_TOOL_LLVM_JITLISTENER_BUILD Off)</font></div><div><font face="Menlo">endif()</font></div></div><div><br></div><div>The problem with keeping the LLVM_EXTERNAL_*_* naming is that LLVM_EXTERNAL_LLVM_JITLISTENER_BUILD makes no sense because it is an in-tree tool. I chose to generalize this to LLVM_TOOL because this handling applies to all processing that occurs from the tools subdirectory. I’m not married to the LLVM_TOOL_* naming convention, but I think LLVM_EXTERNAL is just wrong.</div><div><br></div><div>If your objections to the patches go further than just the naming issue, can you please explain your concerns so that we can see that they are addressed.</div><div><br></div><div>Thank you,</div><div>-Chris</div><div><br></div><div><div><blockquote type="cite"></blockquote></div></div></div><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div>On Jul 8, 2015, at 3:20 PM, Chris Bieneman <<a href="mailto:beanz@apple.com" target="_blank">beanz@apple.com</a>> wrote:</div><br></blockquote></div></div></div><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div><br><blockquote type="cite">On Jul 8, 2015, at 3:06 PM, NAKAMURA Takumi <<a href="mailto:geek4civic@gmail.com" target="_blank">geek4civic@gmail.com</a>> wrote:<br><br>Chris,<br><br>2015-07-09 2:21 GMT+09:00 Chris Bieneman <<a href="mailto:beanz@apple.com" target="_blank">beanz@apple.com</a>>:<br><blockquote type="cite">I was unaware anyone was using that setting through the CMake command line, and there was a corresponding bug in my change that I can address. The other issue is that my patches renamed all the LLVM_EXTERNAL_*_* settings to LLVM_TOOL_*_* because they apply to both in and out of tree tools.<br><br>Is there a reason you use this mechanism? For me to re-land my changes you’ll need to update your bots to use the new setting, or to not rely on this mechanism.<br></blockquote><br>I am sorry that I didn't notice you supposed LLVM_EXTERNAL_*(s) were<br>trivial and I didn't explain since I supposed they were precious.<br>I am investigating bugs around there, later.<br><br><blockquote type="cite">Additionally, your bot doesn’t log its CMake invocation anywhere in the logs that I can see. Can you please update the bot so that it has its CMake invocation logged somewhere on every build so that issues like this can be triaged more easily? Without knowing that you were using the LLVM_EXTERNAL_CLANG_* flags there was no way for me to tell what was going wrong or to reproduce the issue.<br></blockquote><br>Most of my builders are incremental and usually they don't put<br>configurations to the log. I know such a behavior is inconvenient.<br></blockquote><br>I see no problem with the builders being incremental. Can you just log the configuration in the build output? Maybe echo the command line out to the terminal so that people triaging failures know how the build was configured.<br><br><blockquote type="cite"><br>It is doing reconfiguration in every build but it is prioritized<br>lower. <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__bb.pgr.jp_builders_clang-2D3stage-2Dx86-5F64-2Dlinux&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=jHNOlgBUVTch3P7A1vB1GijoKzESE9kg0-WuJyaEp4s&s=1X1rkpAju21t5RbWSItBtbqxBFI1sVELIdC6vybwxG8&e=" target="_blank">http://bb.pgr.jp/builders/clang-3stage-x86_64-linux</a><br>Others are built from clean in every Morning. FYI for example,<br> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__bb.pgr.jp_builders_cmake-2Dclang-2Dx86-5F64-2Dlinux_builds_37092&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=jHNOlgBUVTch3P7A1vB1GijoKzESE9kg0-WuJyaEp4s&s=-1wMMYwyY_3QSgsnJLsTsKJrn87jGuAO2lKIG6zzzOg&e=" target="_blank">http://bb.pgr.jp/builders/cmake-clang-x86_64-linux/builds/37092</a><br> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__bb.pgr.jp_builders_ninja-2Dclang-2Di686-2Dmsc18-2DR_builds_2192&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=jHNOlgBUVTch3P7A1vB1GijoKzESE9kg0-WuJyaEp4s&s=pyJopINwcuzWqOwCpfftgyQQABGyUAGjXB0lJrYZqgY&e=" target="_blank">http://bb.pgr.jp/builders/ninja-clang-i686-msc18-R/builds/2192</a><br><br>FYI, Michael's r155654 was the last change for configuration.<br>I am not a fan of names, LLVM_TOOL_*. I think they are still external.<br></blockquote><br>With my changes this doesn’t just apply to external tools. In fact I want to completely get rid of the distinction of external tool subdirectories in CMake because I don’t think it adds any value.<br><br>Do you have an alternate suggestion for naming that isn’t EXTERNAL? This naming will apply to tools like llc and opt as well as clang and lld. I’m not married to the LLVM_TOOL_* naming, I just felt that LLVM_EXTERNAL_LLC_* would be a terribly misleading setting name.<br><br>Thanks,<br>-Chris<br><br><blockquote type="cite"><br>I won't stop cleanups. CMake scripts are messy.<br><br>...Takumi<br></blockquote><br><br></div></blockquote></div></div></div><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br></div></blockquote></div></div></div></blockquote></div>