This is an archive of the discontinued LLVM Phabricator instance.

Add CMakeLists for External/{Nurbs|Povray|skidmarks10|HMMER}
ClosedPublic

Authored by MatzeB on Nov 10 2015, 6:49 PM.

Details

Summary

Adds a way to configure the location of test-suite externals and adds CMakeLists for Nurbs, Povray, skidmarks10 and HMMER.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 39882.Nov 10 2015, 6:49 PM
MatzeB retitled this revision from to Add CMakeLists for External/{Nurbs|Povray|skidmarks10|HMMER}.
MatzeB updated this object.
MatzeB added reviewers: jmolloy, rengolin, beanz.
MatzeB added a subscriber: llvm-commits.
jmolloy requested changes to this revision.Nov 11 2015, 8:48 AM
jmolloy edited edge metadata.

Hi Matthias,

Thanks for pushing this forward. I've got some comments below.

Cheers,

James

CMakeLists.txt
59 ↗(On Diff #39882)

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

73 ↗(On Diff #39882)

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?

External/CMakeLists.txt
1 ↗(On Diff #39882)

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.

This revision now requires changes to proceed.Nov 11 2015, 8:48 AM
beanz added inline comments.Nov 11 2015, 9:29 AM
CMakeLists.txt
59 ↗(On Diff #39882)

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.

73 ↗(On Diff #39882)

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.

External/CMakeLists.txt
1 ↗(On Diff #39882)

llvm_add_subdirectories actually comes from test-suite, so this is fine.

MatzeB added inline comments.Nov 11 2015, 9:42 AM
CMakeLists.txt
59 ↗(On Diff #39882)

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.

73 ↗(On Diff #39882)

TEST_SUITE_ prefix sounds good.

External/CMakeLists.txt
1 ↗(On Diff #39882)

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
)
MatzeB updated this revision to Diff 40202.Nov 13 2015, 8:33 PM
MatzeB edited edge metadata.

Adapted to review feedback.

This revision was automatically updated to reflect the committed changes.