[llvm-commits] [llvm] r114747 - in /llvm/trunk: CMakeLists.txt cmake/modules/AddLLVM.cmake unittests/CMakeLists.txt utils/unittest/CMakeLists.txt

[Michael Spencer]

On Fri, Sep 24, 2010 at 10:17 PM, Michael Spencer <bigcheesegs at gmail.com> wrote:
> On Fri, Sep 24, 2010 at 9:43 PM, Michael Spencer <bigcheesegs at gmail.com> wrote:
>> On Fri, Sep 24, 2010 at 4:01 PM, Óscar Fuentes <ofv at wanadoo.es> wrote:
>>> Michael Spencer <bigcheesegs at gmail.com>
>>> writes:
>>>
>>>> This was requested for the msvc build bot as building ALL_BUILD from
>>>> VSBuild doesn't build anything and we needed to disable building
>>>> unittests and examples.
>>>
>>> I don't understand this. So if the cmake invocation contains
>>> -DLLVM_BUILD_EXAMPLES=OFF this is a problem?
>>
>> The only way to build (that we could find) clang from the command line
>> using VCBuild is to build the entire solution, which includes the
>> tools, examples, and tests no matter what. LLVM_BUILD_X only controls
>> if they are part of the ALL_BUILD target, and building ALL_BUILD
>> doesn't build anything with VCBuild.
>>
>>> And why you don't want to build examples and unittests on the bot, if
>>> precisely the purpose of the bot is to test the build? (*all* the build,
>>> I guess)
>>
>> This is a clang tester, it slows it down. And the unittests don't
>> build on msvc9 (they do in 10).
>>
>>>> I don't really like the implementation because it may be confusing to
>>>> the user because LLVM_INCLUDE_X is ignored if LLVM_BUILD_X is on. I
>>>> tried getting CMake to update the cache so ccmake and cmake-gui would
>>>> show the change, but it didn't work :(. I would appreciate any help in
>>>> making this implementation more robust.
>>>
>>> I don't like the patch either. The previous implementation supported
>>> ignoring the tools and examples on the default build but allowing
>>> building them later as the user demanded. Now if the user wants to build
>>> a tool or example, he must reconfigure and incorporate *all* the tools
>>> examples to the default build.
>>
>> Ugg, the intention was to not change the behavior at all for TOOLS and
>> EXAMPLES. TOOLS is currently the same, but I accidentally changed
>> EXAMPLES. LLVM_INCLUDE_EXAMPLES should default to ON.
>>
>>> Please explain in more detail what the problem is and maybe we can find
>>> a better implementation.
>>
>> There are two options per group. LLVM_BUILD_X and LLVM_INCLUDE_X.
>> LLVM_BUILD_X says that X should be built by ALL_BUILD, and
>> LLVM_INCLUDE_X says that X should be included as a target that is
>> available to be built. LLVM_BUILD_X must imply LLVM_INCLUDE_X, as you
>> cannot build a target that doesn't exist.
>>
>> The problem is that I could not find a way to change the cache value
>> of LLVM_INCLUDE_X displayed in ccache and cmake-gui to ON if
>> LLVM_BUILD_X is ON. This would make it clearer to the user and make
>> the code simpler.
>>
>>> The current one removes a convenience from the
>>> user
>>
>> It shouldn't. See above.
>>
>>> and is unnecessarily complex (why not
>>>
>>> if( LLVM_INCLUDE_X )
>>>  add_subdirectory(X)
>>> endif()
>>>
>>> instead of changing add_llvm_X)
>>
>> I agree. However, it should be:
>>
>> if( LLVM_INCLUDE_X OR LLVM_BUILD_X )
>>
>> Because of the problem above.
>>
>>> In any case, removing an option (LLVM_BUILD_TOOLS)
>>
>> No options were removed.
>>
>>>  and introducing a new
>>> one (LLVM_INCLUDE_TOOLS) requires updating docs/CMake.html.
>>
>> I agree.
>>
>>> Oh, and next time it would be a good idea to discuss the patch before
>>> committing it.
>>
>> It didn't (well, wasn't supposed to) have any functionality changes,
>> it's rather small, and it was needed to fix the build bot. Is this
>> really too much for review after commit?
>>
>> - Michael Spencer
>>
>
> Attached is a patch that fixes the above (except for auto updating
> LLVM_INCLUDE_X).
>
> - Michael Spencer
>

There was a bug in that last patch. Correct patch attached.

- Michael Spencer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cmake-fix-llvm-include-x.patch
Type: application/octet-stream
Size: 6152 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20100924/b36596f4/attachment.obj>