[Lldb-commits] [PATCH] D64806: [CMake] Always build debugserver on Darwin and allow tests to use the system's one

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.


================
Comment at: lldb/cmake/modules/AddLLDB.cmake:292
+  else()
+    string(STRIP ${xcode_dev_dir} xcode_dev_dir)
+    set(subdir "LLDB.framework/Resources/debugserver")
----------------
Can you add a comment explaining that you want to strip leading whitespace?  Alternatively, if its trailing whitespace, please use `OUTPUT_STRIP_TRAILING_WHITESPACE` in the `execute_process` on L288 please.


================
Comment at: lldb/cmake/modules/AddLLDB.cmake:293
+    string(STRIP ${xcode_dev_dir} xcode_dev_dir)
+    set(subdir "LLDB.framework/Resources/debugserver")
+    set(path_shared "${xcode_dev_dir}/../SharedFrameworks/${subdir}")
----------------
I think that `subdir` is a misleading name.  This is the path to debugserver, `subdir` should be `LLDB.framework/Resources`.  I don't really have an opinion on renaming the variable or composing the variable, that choice is yours :-)


================
Comment at: lldb/tools/debugserver/source/CMakeLists.txt:258
   endif()
-endif()
+#endif()
----------------
sgraenitz wrote:
> Removing the `if() ... endif()` would bloat the diff even more. Would do it in a follow-up NFC commit.
Sure, although ignoring whitespace also is helpful :-)


================
Comment at: lldb/unittests/CMakeLists.txt:83
   add_subdirectory(debugserver)
 endif()
----------------
Shouldn't debugserver not be available always?  It doesn't matter if it isn't being used.  Furthermore, elision from the distribution targets will also prevent the unnecessary build so there is no need to worry about the default builds being longer than necessary.


================
Comment at: lldb/unittests/tools/lldb-server/CMakeLists.txt:16
 
-if(DEBUGSERVER_PATH)
-  add_definitions(-DLLDB_SERVER="${DEBUGSERVER_PATH}" -DLLDB_SERVER_IS_DEBUGSERVER=1)
+if(LLDB_CAN_USE_DEBUGSERVER)
+  if(LLDB_USE_SYSTEM_DEBUGSERVER)
----------------
Why is this being checked in `lldb-server`?


================
Comment at: lldb/unittests/tools/lldb-server/CMakeLists.txt:20
+  else()
+    set(debugserver_path $<TARGET_FILE:lldb-server>)
+  endif()
----------------
Should this not be `debugserver`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64806/new/

https://reviews.llvm.org/D64806