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

[Óscar Fuentes]

Michael Spencer <bigcheesegs at gmail.com>
writes:

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

cmake --build .

on the build directory works fine here, omitting the examples as
required by the setting of LLVM_BUILD_EXAMPLES.

And is it really important to build with VCBuild? NMake works fine and
seems faster than VCBuild. And JOM allows parallel builds, in case the
bot is a multicore machine.

But if you want to speed-up the VS build you could investigate why
tblgen is so slow on Windows. The time it takes to process the files
associated to the X86 target is quite larger than the required for
building the examples.

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

This is a pity, having a bot that ignores part of the build. Apart from
that, failed unittests are significant for clang, aren't they?

[snip]

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

[snip]

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

What we could do is simply

if( LLVM_INCLUDE_TOOLS )
  add_subdirectory(tools)
endif()

and add to the documentation of LLVM_BUILD_TOOLS a note about it being
conditioned by LLVM_INCLUDE_TOOLS (both on the definition of the
variable and on the html file.) Likewise for EXAMPLES.

But all this may be irrelevant if the command "cmake --build ." works
for you as it works for me. In that case simply revert the commit and
you are done.

[snip]

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

Well, you yourself were not satisfied with the change. That is a big
hint that others may share your opinion ;-) so describing the problem
and posting the patch for discussion is a good way of not creating
surprises and even achieving a better solution.