Page MenuHomePhabricator

[CMake] Don't add `cxx` to `LLDB_TEST_DEPS` if it doesn't exist.
Needs RevisionPublic

Authored by delcypher on Feb 5 2019, 6:32 AM.

Details

Summary

If we forget to build libcxx then previously the CMake configure
would have errors like:

CMake Error at
/Users/dan/data/dev/llvm/upstream_monorepo/master/llvm/llvm/cmake/modules/AddLLVM.cmake:1374
(add_dependencies):
  The dependency target "cxx" of target
    "check-lldb-tools-lldb-mi-data-inputs" does not exist.
    Call Stack (most recent call first):
      /Users/dan/data/dev/llvm/upstream_monorepo/master/llvm/llvm/cmake/modules/AddLLVM.cmake:1426
      (add_lit_target)
        /Users/dan/data/dev/llvm/upstream_monorepo/master/llvm/lldb/lit/CMakeLists.txt:76
        (add_lit_testsuites)

To avoid error this check the cxx target exists before adding it to
LLDB_TEST_DEPS. If it doesn't exist emit a warning similar to the
code above.

Event Timeline

delcypher created this revision.Feb 5 2019, 6:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 6:32 AM
Herald added a subscriber: mgorny. · View Herald Transcript

Wow phabricator has made a right mess of displaying this patch. It's easier to look at as.

diff --git a/lldb/CMakeLists.txt b/lldb/CMakeLists.txt
index fdd84a6e7b7..6b26adb3383 100644
--- a/lldb/CMakeLists.txt
+++ b/lldb/CMakeLists.txt
@@ -111,7 +111,11 @@ if(LLDB_INCLUDE_TESTS)
           message(WARNING "LLDB test suite requires libc++ in llvm/projects/libcxx or an existing build symlinked to ${cxx_dir}")
         endif()
       else()
-        list(APPEND LLDB_TEST_DEPS cxx)
+        if (NOT TARGET cxx)
+          message(WARNING "LLDB test suite requires libc++")
+        else()
+          list(APPEND LLDB_TEST_DEPS cxx)
+        endif()
       endif()
     endif()
   endif()

Not sure if we want to change the behavior this way. The purpose of this code was to make the dependency to libc++ explicit because it's specific to macOS and it's missed quite often. In my experience warnings like the proposed one are hard to recognize in the load of CMake output.

Note that you can configure with LLDB_INCLUDE_TESTS=OFF to avoid the libc++ dependency. We could have an explicit error message that points this out. What do you think?

JDevlieghere requested changes to this revision.Feb 5 2019, 10:49 AM

Agreed with Stefan, I don't think this should be a warning.

This revision now requires changes to proceed.Feb 5 2019, 10:49 AM