[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 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cmake-fix-llvm-include-x.patch
Type: application/octet-stream
Size: 5263 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20100924/60c3fe93/attachment.obj>