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

[arrowdodger]

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