[Lldb-commits] [PATCH] D46334: [CMake] Unify and relayer testing

[Stella Stamenova via Phabricator via lldb-commits]

stella.stamenova added a comment.

I am going to run the tests after you make the last few changes. I want to make sure the compiler path is no longer being overwritten by the test cmake before I run them.



================
Comment at: test/CMakeLists.txt:138
 if(CMAKE_HOST_APPLE)
-  list(APPEND LLDB_TEST_COMMON_ARGS --server ${DEBUGSERVER_PATH})
+  if (DEBUGSERVER_PATH STREQUAL "$<TARGET_FILE:debugserver>")
+    list(APPEND LLDB_EXECUTABLE_PATH_ARGS --server ${DEBUGSERVER_PATH})
----------------
stella.stamenova wrote:
> JDevlieghere wrote:
> > stella.stamenova wrote:
> > > I am wondering if it's possible to make the logic here simpler. Since we need to handle each of the properties that can contain TARGET_FILE, it is starting to get rather complicated
> > > 
> > > I think you can actually use LLDB_DOTEST_ARGS_STR for the check-lldb-single target (since it creates a project that should be correctly substituted), so the only other place that could create issues is the lldb-dotest script. Since the tests are run as part of lit now, do we still need the lldb-dotest script?
> > > 
> > I started out that route, but how would that work for `check-lldb-single` with multiple targets? I'm not worried about `lldb-dotest`, we can do the same trick as for `llvm-lit`(see llvm/utils/llvm-lit/CMakeLists.txt). 
> I cannot speak for other generators that support multiple configurations (though I suspect it is similar), but for Visual Studio, if the path to a tool contained CMAKE_CFG_INTDIR (which is $(Configuration)), when the project was built, $(Configuration) would be replaced with the configuration that is currently being used which is exactly what we want anyway.
> 
> Obviously, some additional testing will be required to verify that this is the case always.
Why not change the path where it is set originally? Then you won't need the if/else here.


================
Comment at: test/CMakeLists.txt:49
 
-# We need two properties here, because they are used for different purposes. When we are generating
-# one file per configuration for lldb-dotest, we want the paths to be configuration specific. However,
-# when we are generating a single lit file, the file itself should not be per configuration and the paths
-# contained inside should be generic also.
 set(LLDB_EXECUTABLE_PATH_ARGS
   --executable ${LLVM_RUNTIME_OUTPUT_INTDIR}/lldb${CMAKE_EXECUTABLE_SUFFIX}
----------------
I think you can get rid of LLDB_EXECUTABLE_PATH_ARGS now and just set these properties in LDLB_TEST_COMMON_ARGS instead.


================
Comment at: test/CMakeLists.txt:56
 if (NOT LLDB_TEST_USE_CUSTOM_C_COMPILER)
-  list(APPEND LLDB_EXECUTABLE_PATH_ARGS -C $<TARGET_FILE:clang>)
+  list(APPEND LLDB_EXECUTABLE_PATH_ARGS -C ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang${CMAKE_EXECUTABLE_SUFFIX})
 else()
----------------
You actually don't need this anymore. When this was being created per configuration, we needed to overwrite the value of the compiler, but regardless of whether it is custom, it will point to the correct path. You can remove the entire if/else.


https://reviews.llvm.org/D46334