[Lldb-commits] [PATCH] D56400: [CMake] Some cleanup around test preparations

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz marked 7 inline comments as done.
sgraenitz added inline comments.


================
Comment at: CMakeLists.txt:43
 if(LLDB_INCLUDE_TESTS)
+  # FIXME: LLDB_TEST_CXX_COMPILER is unused.
+  # FIXME: In standalone builds LLVM_BINARY_DIR points to LLDB's build directory and not to LLVM's!
----------------
sgraenitz wrote:
> labath wrote:
> > JDevlieghere wrote:
> > > Why is this unused? Are we only using `LLDB_TEST_C_COMPILER` and if so, why?
> > I think that's because this only gets passed to dotest, that it has it's own logic for deducing the C++ compiler name.
> Yes, we only ever pass on `LLDB_TEST_C_COMPILER` and it should be sufficient.
> https://github.com/llvm-mirror/lldb/blob/2b03c2512ebf1999e330e85d60b7f93703483485/test/CMakeLists.txt#L53
> 
> https://lldb.llvm.org/build.html reads: //Note that MSVC is not supported here, it must be a path to a clang executable.//
> As clang == clang++ ⇒ q.e.d :)
> 
> I think what we really want here is a `LLDB_TEST_COMPILER` setting, but this means: update docs and build bots, potentially phase out the setting? I am not sure about their popularity.
There was a previous initiative for cleanup in D39215. My follow-up patch D56440 aims to finish this.


================
Comment at: CMakeLists.txt:44
+  # FIXME: LLDB_TEST_CXX_COMPILER is unused.
+  # FIXME: In standalone builds LLVM_BINARY_DIR points to LLDB's build directory and not to LLVM's!
+  # FIXME: LLDB_TEST_USE_CUSTOM_C/CXX_COMPILER options don't work as intended!
----------------
sgraenitz wrote:
> JDevlieghere wrote:
> > sgraenitz wrote:
> > > This was here already when the file was created:
> > > https://github.com/llvm-mirror/lldb/blob/98c187f2613fd8e2f5cdf5f8f35503a102a54f11/cmake/modules/LLDBStandalone.cmake#L67
> > > 
> > > Apparently the below default paths for dsymutil, FileCheck and clang never worked in standalone builds..
> > Instead of this FIXME, should we explicitly unset this variable to prevent mistakes in the future? Or is this fixable?
> Good point. Not sure what the consequences would be. I will try and see what happens.
See followup patch D56443.


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

https://reviews.llvm.org/D56400