[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 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