[LLVMdev] Some CMake issues (Are you being served?)

[Stephen Kelly]

NAKAMURA Takumi wrote:

> 
>> 4) There seems to be a 'cmake package config file' generated by the llvm
>> build.
>>
>>  http://www.cmake.org/cmake/help/git-master/manual/cmake-packages.7.html#package-configuration-file
>>
>>  (the doc is new in master, but it applies almost entirely to CMake 2.8.8
>> too)
>>
>> but it doesn't seem to appear in my llvm-3.5-dev ubuntu package.
>>
>> You need to ensure that the package config file is generated by your
>> buildsystem. If you support two buildsystems, you need to ensure that it
>> is generated properly by both of them. As far as I can see, the Makefile
>> buildsystem in llvm does not generate the files. This is a significant
>> bug. It is equivalent to not generating a required header file in one of
>> your supported buildsystems.
>>
>> You need to ensure that the config file is generated by both
>> buildsystems. That can mean that you avoid the built-in CMake facilities
>> for creating packages.
>>
>> [Sidebar: This is part of the reason why I strongly recommend any project
>> [to
>> have only one buildsystem. If it is impossible for you to drop the
>> Makefile system, then consider dropping the CMake one. It creates false
>> expectations of ability to use packages downstream.]
> 
> Brad King is proposing the stuff. Excuse me we haven't accepted his yet.

Oh, I didn't know Brad was also working on this stuff in llvm. Good. I 
haven't reviewed his patches, but if it's generating the same content with 
CMake and Makefiles, that sounds good.

> See also http://llvm.org/bugs/show_bug.cgi?id=15732

Some of the bugs are questionable or unclear. I've left some comments.

> 
>> 5) When I see things like this:
>>
>>  add_dependencies(clangStaticAnalyzerCheckers
>>    ClangAttrClasses
>>    ClangAttrList
>>    ClangCommentNodes
> 
> They give dependencies for generated headers. What'd be wrong to you?

Oh, ok. I didn't know what they were for, and as I've seen other code for 
manually managing dependencies, I thought this was more of the same.

>> and this:
>>
>>  set(LLVM_LINK_COMPONENTS
>>   Support
>>  )
> 
> LLVM_LINK_COMPONENTS is for compatibility of autoconf build.
> In AddLLVM.cmake and LLVM-Config.cmake, it is translated to
> target_link_libraries.

What compatibility? A file is generated by a cmake llvm build for autoconf-
using downstreams to use, or something?

>> I don't know what those lines are for, but it looks like 'you're doing it
>> wrong' from a dependency specification point of view, or CMake is not
>> giving you the interfaces to do it right. If it's the latter, I want to
>> fix that.
> 
> You may suppose the former is coming from historical reason. Please be
> patient.

Sorry, I did not intend to appear impatient. I'm more looking for gaps in 
cmake than in the llvm buildsystem.

> The latter, I am certain CMake is insufficient.
> I know it'd be not easy to describe optimal behavior for generated
> headers.
> 
> FYI, ClangXXX is doing;
> 
>   1. Generate clang-tblgen.
>   2. Generate foo.inc from foobar.td.
>   3. Build object files with generated foo.inc.
> 
> Do you think we should give suboptimal dependency,
> "Build all files whenever any of *.td were touchd"?

Maybe this will be easier to work on after Brads patches are in.

> 
>> 6) If you create a proper config file, then you can populate it with
>> IMPORTED targets and use it in clang. IMPORTED targets record
>> dependencies and usage requirements (when you start requiring CMake
>> 2.8.9+ at least) properly.
>>
>>  http://www.cmake.org/cmake/help/git-master/manual/cmake-buildsystem.7.html#imported-targets
> 
> Most of us are building clang in llvm build tree.
> Aka "standalone clang" could be supposed as w-i-p.
> I think other projects could be good studies for external cmake build.
> 
> I agree to update cmake version if LLVMConfig.cmake would work more smart.
> ;)

Actually, CMake 2.8.11 would be a better minimum to aim for. It handles 
transitive usage requirements, but earlier versions do not. I was wrong 
about 2.8.9.

Anyway, from the description, Brads patches add IMPORTED targets, so let's 
have another look when they are in.

Thanks,

Steve.