[Lldb-commits] [PATCH] D71306: [RFC] Change how we deal with optional dependencies

[Pavel Labath via Phabricator via lldb-commits]

labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.

I like where this is going, but I think this still needs some work wrt. the panel library (and being able to customize the dependency search more).



================
Comment at: lldb/cmake/modules/LLDBConfig.cmake:28
+function(add_optional_dependency variable description package found)
+  set(${variable} "AUTO" CACHE STRING "${description} On, Off or Auto (default)")
+  string(TOUPPER "${${variable}}" ${variable})
----------------
Use the same capitalization of "auto" as the description


================
Comment at: lldb/cmake/modules/LLDBConfig.cmake:41-47
+  # We could set ${variable} directory to ${${found}} but then the value is
+  # TRUE/FALSE rather than ON/OFF.
+  if (${${found}})
+    set(${variable} ON PARENT_SCOPE)
+  else()
+    set(${variable} OFF PARENT_SCOPE)
+  endif()
----------------
I don't care that much about that, but I don't see the advantage in canonicalizing to ON/OFF (the other variables don't go through the canonicalization process, so only their default values will be ON/OFF -- the actual values will be whatever flavour of "true" the user chooses to pass to cmake)


================
Comment at: lldb/cmake/modules/LLDBConfig.cmake:507
     if (NOT CURSES_PANEL_LIBRARY)
-        message(FATAL_ERROR "A required curses' panel library not found.")
+      set(LLDB_ENABLE_CURSES OFF)
+      message(WARNING "Curses panel library not found.")
----------------
I was wondering above if the name we pass to the `find_package` provides sufficient customization, and I think this shows it doesn't. Ideally, the panel (sub)library would behave the same way as the curses library and respect all three values of the cache variable. This one does not support the "force on" mode.

Having the cache variable handling code centralized in a single function is a worthy goal, but I am not sure that this is really possible, given the current cmake abilities. e.g. we'd need some sort of a indirect call so we can implement custom searching logic, but there aren't any particularly nice ways of achieving that. The only way I know of achieving indirection is to put the logic in a separate file, and then use either an `include`, `find_package` or something else to load it.

If you want to go the file loading approach, or "outline" than function (at least some parts of it), then I think both are fine, but I don't think leaving this code in here is good, since the whole point of this exercise is to standardize things.


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

https://reviews.llvm.org/D71306