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

[NAKAMURA Takumi]

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.

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

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

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

...Takumi

On Fri, Jul 10, 2015 at 2:37 AM Chris Bieneman <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> wrote:
>
>
> On Jul 8, 2015, at 3:06 PM, NAKAMURA Takumi <geek4civic at gmail.com> wrote:
>
> Chris,
>
> 2015-07-09 2:21 GMT+09:00 Chris Bieneman <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
> 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/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
> 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/c5bd06d1/attachment.html>