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

[arrowdodger]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120112/34191dd3/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/20120112/34191dd3/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/20120112/34191dd3/attachment-0001.bin>