[PATCH] D14561: Add CMakeLists for External/{Nurbs|Povray|skidmarks10|HMMER}

[Matthias Braun via llvm-commits]

MatzeB added inline comments.

================
Comment at: CMakeLists.txt:59
@@ +58,3 @@
+  if(CMAKE_SIZEOF_VOID_P EQUAL 8)
+    set(ARCH_LP64 TRUE CACHE BOOL "Whether target architecture has 64bit pointers")
+  endif()
----------------
beanz wrote:
> jmolloy wrote:
> > Don't we want something like this instead?:
> > 
> >   if(CMAKE_SIZEOF_VOID_P EQUAL 8)
> >     set(TMP_LP64 TRUE)
> >   else()
> >     set(TMP_LP64 FALSE)
> >   endif()
> >   set(ARCH_LP64 ${TMP_LP64} CACHE BOOL "Does the target architecture has 64-bit pointers?")
> > 
> > I'll defer to Chris B on this
> Why does this need to be cached at all?
> 
> I generally prefer not using cached variables unless there is a really good reason because CMake has no cache invalidation.
My goal here was not so much the caching but more about giving the user control over these settings. Though I guess the proper design would be to only give a variable to the user to override the auto detection instead of saving the result of the auto detection in the cache.

Anyway I agree that in this specific instance it's probably best to not make it cached at all.

================
Comment at: CMakeLists.txt:73
@@ -65,1 +72,3 @@
 add_subdirectory(MultiSource)
+if(EXTERNALS_DIR)
+	add_subdirectory(External)
----------------
beanz wrote:
> jmolloy wrote:
> > I'd prefer if we had a "LLVM_" prefix on all variables we expect to be set by the user. Can we stick to that, like in the other projects' builds?
> Rather than LLVM, I would suggest prefixing with TEST_SUITE.
> 
> I think each project should have its own prefix. Also, with r252747 variables that start with TEST_SUITE will automatically be passed from the top-level CMake invocation into the sub-project configuration.
TEST_SUITE_ prefix sounds good.

================
Comment at: External/CMakeLists.txt:1
@@ +1,1 @@
+llvm_add_subdirectories(HMMER Nurbs Povray skidmarks10)
----------------
beanz wrote:
> jmolloy wrote:
> > I think this relies on a CMake module from LLVM, not from test-suite, so it'll fail unless we're in /projects.
> > 
> > Just doing "add_subdirectory()" several times isn't much harder.
> llvm_add_subdirectories actually comes from test-suite, so this is fine.
I can change the layout to this to give more of a "one-item in each line" feeling which should be good for subdirectories.

```
llvm_add_subdirectories(
  HMMER
  Nurbs
  Povray
  skidmarks10
)
```



http://reviews.llvm.org/D14561