[llvm-commits] [PATCH][CMake] PR10050.

[arrowdodger]

On Thu, Jan 12, 2012 at 6:28 PM, arrowdodger <6yearold at gmail.com> wrote:

> On Sun, Jan 8, 2012 at 3:36 PM, arrowdodger <6yearold at gmail.com> wrote:
>
>> On Mon, Jan 2, 2012 at 9:36 PM, Chandler Carruth <chandlerc at google.com>wrote:
>>
>>> Thanks, but there are some real problems with how this is structured.
>>> The first two are easy to fix, but the last may not be.
>>>
>>> 1) The macros are strangely formed in a few ways. First, please avoid
>>> magical arrays filled in with the results of your macro. They're very hard
>>> to understand. I'd much rather that each macro produces a new target which
>>> other things can then depend on, or populates an array the name of which is
>>> passed into the macro. Also, why make the user write a foreach loop around
>>> the files? It seems like you could have them provide a list of files rather
>>> than a single file, and do the looping inside the macro. Finally, please
>>> allow the user to specify the actual filename rather than the basename of
>>> the podfile. Compute the basename yourself, as this will make both the use
>>> of the macro easier to understand and it's implementation more clear.
>>>
>>
>> Does it looks better now?
>>
>>
>>> 2) Please use something more distinct such as 'llvm-docs-*' as the
>>> custom target names; 'llvm-ps' looks too much like it could be one of the
>>> actual LLVM tools.
>>>
>>
>> Done.
>>
>>
>>> 3) I don't understand why you can't use normal CMake dependencies to
>>> connect the install rules to the actual build steps. It seems like you
>>> could use install(FILES ${HTML_FILES} ...) or some equivalent construct.
>>> CMake should be handling the dependency computations and ensuring that
>>> those files are build for 'make install'.
>>>
>>
>> Yeah, it should. But it does not. I've even asked on StackOverflow about
>> this.
>>
>>
>>> If CMake cannot do this naturally with its own dependency information,
>>> then I fear it may not be worth adding this to the CMake builds, and they
>>> will be less functional than the autotools builds. I think re-executing the
>>> make tool is an unacceptable design wart.
>>>
>>
>> Why do you think so? Isn't that hack is what CMake should actually do -
>> just call `make <target>` during `make install`? If CMake fix it, this hack
>> can be removed by altering only 3 lines in AddLLVM.cmake.
>>
>> I also have trouble believing CMake can't represent this pattern, but I
>>> haven't tried my hand at it...
>>>
>>
>> If you find cleaner way to solve this, i would be happy to redo the patch.
>>
>
> Bump.
>

Another bump.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120119/5a3f63a2/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang.cmake.docs.patch
Type: text/x-patch
Size: 4064 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120119/5a3f63a2/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm.cmake.docs.patch
Type: text/x-patch
Size: 8875 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120119/5a3f63a2/attachment-0001.bin>