<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="">How does this sound. I’d like to re-land the patches with the following changes:</div><div class=""><br class=""></div><div class="">(1) Keep the LLVM_EXTERNAL_*_SOURCE_DIR naming</div><div class="">(2) Ignore LLVM_EXTERNAL_*_SOURCE_DIR if an in-tree directory exists</div><div class="">(3) Only cache LLVM_EXTERNAL_*_SOURCE_DIR variables if they are used</div><div class=""><br class=""></div><div class="">The patches will still:</div><div class="">(1) Migrate to implicit adding tools by default</div><div class="">(2) Use LLVM_TOOL_*_BUILD to enable and disable tool builds, and to prevent tools from being added more than once</div><div class="">(3) Generate LLVM_TOOL_*_BUILD for all tools as advanced options</div><div class=""><br class=""></div><div class="">Thanks,</div><div class="">-Chris</div><div class=""><br class=""></div><div class=""><div><blockquote type="cite" class=""><div class="">On Jul 9, 2015, at 4:16 PM, Chris Bieneman <<a href="mailto:beanz@apple.com" class="">beanz@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" class=""><div class=""><br class="Apple-interchange-newline">On Jul 9, 2015, at 4:05 PM, NAKAMURA Takumi <<a href="mailto:geek4civic@gmail.com" class="">geek4civic@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">Chris,<br class=""><br class="">I am still not a fan of LLVM_TOOL_CLANG_*. It's external for me.<div class="">I suppose generic naming rule is not a matter.</div><div class=""><br class=""></div><div class="">I describe an alternate idea. Could you consider?</div><div class=""><br class=""></div><div class="">1) Don't generate LLVM_TOOL_*_SOURCE_DIR. Assume in-tree tools are not relocatable.</div></div></div></blockquote><div class=""><br class=""></div><div class="">Yes, we can ignore overridden source directories for tools that exist in-tree. This is different from the current behavior. Today if you have clang in tree and use LLVM_EXTERNAL_CLANG_SOURCE_DIR to point to a different clang, it overrides the in-tree one.</div><div class=""><br class=""></div><div class="">If nobody objects to the behavior change I can take this route instead. If we do that then the LLVM_TOOL vs LLVM_EXTERNAL naming issue matters less (assuming we do change LLVM_EXTERNAL_*_BUILD to LLVM_TOOL_*BUILD).</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><br class=""></div><div class="">2) If corresponding source directory of each tool isn't present, let in-tree stuff do nothing.</div></div></div></blockquote><div class=""><br class=""></div><div class="">You mean that if LLVM_EXTERNAL_CLANG_SOURCE_DIR points to an empty directory CMake should ignore it? That would be inline with the current behavior.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><br class=""></div><div class="">3) external stuff may be suppressed if corresponding in-tree directory were there. (optional)</div></div></div></blockquote><div class=""><br class=""></div><div class="">As I mention above, this is opposite to the current behavior.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><br class=""></div><div class="">I have a suggestion around LLVM_TOOL_*.</div><div class="">Dozen of auto-generated LLVM_TOOL_*_BUILD might be nasty.</div><div class="">How about LLVM_TOOLS_TO_BUILD=llc;lli;llvm-as.... ?</div></div></div></blockquote><div class=""><br class=""></div><div class="">I think it is much cleaner inside CMake to use compassable variables. It means you are not searching a list. I’m not opposed to supplying LLVM_TOOLS_TO_BUILD as a convenience wrapper that populates LLVM_TOOL_*_BUILD, but I think using LLVM_TOOL_*_BUILD is a much cleaner interface inside CMake.</div><div class=""><br class=""></div><div class="">-Chris</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><br class=""></div><div class="">...Takumi</div></div><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Fri, Jul 10, 2015 at 2:37 AM Chris Bieneman <<a href="mailto:beanz@apple.com" class="">beanz@apple.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div class="" style="word-wrap: break-word;">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 class=""><blockquote type="cite" class=""></blockquote></div></div></div><div class="" style="word-wrap: break-word;"><div class=""><div class=""><blockquote type="cite" class=""><div class="">On Jul 8, 2015, at 3:20 PM, Chris Bieneman <<a href="mailto:beanz@apple.com" target="_blank" class="">beanz@apple.com</a>> wrote:</div><br class=""></blockquote></div></div></div><div class="" style="word-wrap: break-word;"><div class=""><div class=""><blockquote type="cite" class=""><div class=""><br class=""><blockquote type="cite" class="">On Jul 8, 2015, at 3:06 PM, NAKAMURA Takumi <<a href="mailto:geek4civic@gmail.com" target="_blank" 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" target="_blank" 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.<span class="Apple-converted-space"> </span><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=0hr6zXWSXFyyXNi2e1cnF86qjwuc0odV22KAvgTHTtk&s=dFv_tsEWRiKJs1O2os4GzvmCLrumGl5hzIwsWsr5q-8&e=" target="_blank" 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=0hr6zXWSXFyyXNi2e1cnF86qjwuc0odV22KAvgTHTtk&s=CIPvM6AZGXaQNG-GOF5-S8N2Y4JZ6vGfoQZG6XsmziA&e=" target="_blank" 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=0hr6zXWSXFyyXNi2e1cnF86qjwuc0odV22KAvgTHTtk&s=l46ezL596KgTzOBh0CjJEF-Pk3QG8OkRspExuIKaGFg&e=" target="_blank" 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=""></div></blockquote></div></div></div><div class="" style="word-wrap: break-word;"><div class=""><div class=""><blockquote type="cite" class=""><div class="">_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank" class="">llvm-commits@cs.uiuc.edu</a><br class=""><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank" class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br class=""></div></blockquote></div></div></div></blockquote></div></div></blockquote></div><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">_______________________________________________</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">llvm-commits mailing list</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="mailto:llvm-commits@cs.uiuc.edu" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">llvm-commits@cs.uiuc.edu</a><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></div></blockquote></div><br class=""></div></body></html>