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

[Chandler Carruth]

On Thu, Dec 29, 2011 at 8:09 AM, arrowdodger <6yearold at gmail.com> wrote:

> Updated patches attached. Changes:
> 1. All generating targets are now running only when user is invoking
> install rule. Credits go to @sakra on SO.
> 2. Wrapped some common code into macros, put them into
> cmake/modules/AddLLVM.cmake.
> 3. Create .tar.gz's only if both tar and gzip are found, otherwise warn
> user and don't even generate targets.
> 4. Code style fixes.


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.

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.

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'. 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. I also have trouble believing CMake can't represent this
pattern, but I haven't tried my hand at it...
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120102/946560bb/attachment.html>