[llvm] r241663 - Revert r241621, "[CMake] Cleanup tools/CMakeLists.txt to take advantage of the auto-registration that was already partially working."

Thu Jul 9 16:16:05 PDT 2015

> On Jul 9, 2015, at 4:05 PM, NAKAMURA Takumi <geek4civic at gmail.com> wrote:
> 
> Chris,
> 
> I am still not a fan of LLVM_TOOL_CLANG_*. It's external for me.
> I suppose generic naming rule is not a matter.
> 
> I describe an alternate idea. Could you consider?
> 
> 1) Don't generate LLVM_TOOL_*_SOURCE_DIR. Assume in-tree tools are not relocatable.

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.

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).

> 
> 2) If corresponding source directory of each tool isn't present, let in-tree stuff do nothing.

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.

> 
> 3) external stuff may be suppressed if corresponding in-tree directory were there. (optional)

As I mention above, this is opposite to the current behavior.

> 
> I have a suggestion around LLVM_TOOL_*.
> Dozen of auto-generated LLVM_TOOL_*_BUILD might be nasty.
> How about LLVM_TOOLS_TO_BUILD=llc;lli;llvm-as.... ?

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.

-Chris

> 
> ...Takumi
> 
> On Fri, Jul 10, 2015 at 2:37 AM Chris Bieneman <beanz at apple.com <mailto:beanz at apple.com>> wrote:
> Takumi,
> 
> 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.
> 
> 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.
> 
> 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.
> 
> 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).
> 
> I chose to eliminate the differences by extending add_llvm_external_project to be the “one true way” to make this work.
> 
> With my patches the following code in tools/CMakeLists.txt:
> 
> if( LLVM_USE_INTEL_JITEVENTS )
>   add_llvm_tool_subdirectory(llvm-jitlistener)
> else()
>   ignore_llvm_tool_subdirectory(llvm-jitlistener)
> endif( LLVM_USE_INTEL_JITEVENTS )
> 
> Simplifies to:
> 
> if(NOT LLVM_USE_INTEL_JITEVENTS )
>   set(LLVM_TOOL_LLVM_JITLISTENER_BUILD Off)
> endif()
> 
> 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.
> 
> 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.
> 
> Thank you,
> -Chris
> 
> 
>> On Jul 8, 2015, at 3:20 PM, Chris Bieneman <beanz at apple.com <mailto:beanz at apple.com>> wrote:
>> 
> 
>> 
>>> On Jul 8, 2015, at 3:06 PM, NAKAMURA Takumi <geek4civic at gmail.com <mailto:geek4civic at gmail.com>> wrote:
>>> 
>>> Chris,
>>> 
>>> 2015-07-09 2:21 GMT+09:00 Chris Bieneman <beanz at apple.com <mailto:beanz at apple.com>>:
>>>> 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.
>>>> 
>>>> 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.
>>> 
>>> I am sorry that I didn't notice you supposed LLVM_EXTERNAL_*(s) were
>>> trivial and I didn't explain since I supposed they were precious.
>>> I am investigating bugs around there, later.
>>> 
>>>> 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.
>>> 
>>> Most of my builders are incremental and usually they don't put
>>> configurations to the log. I know such a behavior is inconvenient.
>> 
>> 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.
>> 
>>> 
>>> It is doing reconfiguration in every build but it is prioritized
>>> lower. http://bb.pgr.jp/builders/clang-3stage-x86_64-linux <http://bb.pgr.jp/builders/clang-3stage-x86_64-linux>
>>> Others are built from clean in every Morning. FYI for example,
>>> http://bb.pgr.jp/builders/cmake-clang-x86_64-linux/builds/37092 <http://bb.pgr.jp/builders/cmake-clang-x86_64-linux/builds/37092>
>>> http://bb.pgr.jp/builders/ninja-clang-i686-msc18-R/builds/2192 <http://bb.pgr.jp/builders/ninja-clang-i686-msc18-R/builds/2192>
>>> 
>>> FYI, Michael's r155654 was the last change for configuration.
>>> I am not a fan of names, LLVM_TOOL_*. I think they are still external.
>> 
>> 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.
>> 
>> 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.
>> 
>> Thanks,
>> -Chris
>> 
>>> 
>>> I won't stop cleanups. CMake scripts are messy.
>>> 
>>> ...Takumi
>> 
>> 
> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150709/2f0eec65/attachment.html>