<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Takumi,<div class=""><br class=""></div><div class="">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 class=""><br class=""></div><div class="">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 class=""><br class=""></div><div class="">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 class=""><br class=""></div><div class="">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 class=""><br class=""></div><div class="">I chose to eliminate the differences by extending add_llvm_external_project to be the “one true way” to make this work.</div><div class=""><br class=""></div><div class="">With my patches the following code in tools/CMakeLists.txt:</div><div class=""><br class=""></div><div class=""><div class=""><font face="Menlo" class="">if( LLVM_USE_INTEL_JITEVENTS )</font></div><div class=""><font face="Menlo" class=""> add_llvm_tool_subdirectory(llvm-jitlistener)</font></div><div class=""><font face="Menlo" class="">else()</font></div><div class=""><font face="Menlo" class=""> ignore_llvm_tool_subdirectory(llvm-jitlistener)</font></div><div class=""><font face="Menlo" class="">endif( LLVM_USE_INTEL_JITEVENTS )</font></div></div><div class=""><br class=""></div><div class="">Simplifies to:</div><div class=""><br class=""></div><div class=""><div class=""><font face="Menlo" class="">if(NOT LLVM_USE_INTEL_JITEVENTS )</font></div><div class=""><font face="Menlo" class=""> set(LLVM_TOOL_LLVM_JITLISTENER_BUILD Off)</font></div><div class=""><font face="Menlo" class="">endif()</font></div></div><div class=""><br class=""></div><div class="">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 class=""><br class=""></div><div class="">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 class=""><br class=""></div><div class="">Thank you,</div><div class="">-Chris</div><div class=""><br class=""></div><div class=""><div><blockquote type="cite" class=""><div class="">On Jul 8, 2015, at 3:20 PM, Chris Bieneman <<a href="mailto:beanz@apple.com" class="">beanz@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><br class=""><blockquote type="cite" class="">On Jul 8, 2015, at 3:06 PM, NAKAMURA Takumi <<a href="mailto:geek4civic@gmail.com" class="">geek4civic@gmail.com</a>> wrote:<br class=""><br class="">Chris,<br class=""><br class="">2015-07-09 2:21 GMT+09:00 Chris Bieneman <<a href="mailto:beanz@apple.com" class="">beanz@apple.com</a>>:<br class=""><blockquote type="cite" class="">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 class=""><br class="">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 class=""></blockquote><br class="">I am sorry that I didn't notice you supposed LLVM_EXTERNAL_*(s) were<br class="">trivial and I didn't explain since I supposed they were precious.<br class="">I am investigating bugs around there, later.<br class=""><br class=""><blockquote type="cite" class="">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 class=""></blockquote><br class="">Most of my builders are incremental and usually they don't put<br class="">configurations to the log. I know such a behavior is inconvenient.<br class=""></blockquote><br class="">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 class=""><br class=""><blockquote type="cite" class=""><br class="">It is doing reconfiguration in every build but it is prioritized<br class="">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=2FPjXsH1Rpi8Elgls4g1NaA5eDGs7ulpkJLHxvR42V0&s=0m_S_2EcOjU6QZxX0wdnQoxNWPqg4LptbrVFjlAZ2zc&e=" class="">http://bb.pgr.jp/builders/clang-3stage-x86_64-linux</a><br class="">Others are built from clean in every Morning. FYI for example,<br class=""> <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=2FPjXsH1Rpi8Elgls4g1NaA5eDGs7ulpkJLHxvR42V0&s=qrY1bMU0Dle4VQ6M9hJfGcMRqCwptbCaQmlI51VCOw8&e=" class="">http://bb.pgr.jp/builders/cmake-clang-x86_64-linux/builds/37092</a><br class=""> <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=2FPjXsH1Rpi8Elgls4g1NaA5eDGs7ulpkJLHxvR42V0&s=KJ6LY5c0YDQTYO9Zn3smGpm3YRahNkhXzT8sdfm5yiQ&e=" class="">http://bb.pgr.jp/builders/ninja-clang-i686-msc18-R/builds/2192</a><br class=""><br class="">FYI, Michael's r155654 was the last change for configuration.<br class="">I am not a fan of names, LLVM_TOOL_*. I think they are still external.<br class=""></blockquote><br class="">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 class=""><br class="">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 class=""><br class="">Thanks,<br class="">-Chris<br class=""><br class=""><blockquote type="cite" class=""><br class="">I won't stop cleanups. CMake scripts are messy.<br class=""><br class="">...Takumi<br class=""></blockquote><br class=""><br class="">_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@cs.uiuc.edu" class="">llvm-commits@cs.uiuc.edu</a><br class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br class=""></div></blockquote></div><br class=""></div></body></html>