[PATCH] D109625: [compiler-rt] Ensure required deps for tests targets are actually built

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

Hmm. Just found out about this, but for future reference, I think this will 
only work for `ninja check-runtimes` and `ninja check-runtimes-{target}`, but 
not necessarily `ninja -C runtimes/runtimes-{target} 
check-runtimes/check-compilter-rt`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109625

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109625: [compiler-rt] Ensure required deps for tests targets are actually built

[Leonard Chan via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG79b422080612: [runtimes] Ensure required deps for tests 
targets are actually built (authored by leonardchan).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109625

Files:
  llvm/runtimes/CMakeLists.txt


Index: llvm/runtimes/CMakeLists.txt
===
--- llvm/runtimes/CMakeLists.txt
+++ llvm/runtimes/CMakeLists.txt
@@ -294,6 +294,7 @@
 set(runtimes-test-depends-${name} runtimes-test-depends)
 set(check-runtimes-${name} check-runtimes)
 list(APPEND ${name}_test_targets runtimes-test-depends-${name} 
check-runtimes-${name})
+list(APPEND test_targets ${${name}_test_targets})
 foreach(target_name IN LISTS SUB_CHECK_TARGETS)
   set(${target_name}-${name} ${target_name})
   list(APPEND ${name}_test_targets ${target_name}-${name})


Index: llvm/runtimes/CMakeLists.txt
===
--- llvm/runtimes/CMakeLists.txt
+++ llvm/runtimes/CMakeLists.txt
@@ -294,6 +294,7 @@
 set(runtimes-test-depends-${name} runtimes-test-depends)
 set(check-runtimes-${name} check-runtimes)
 list(APPEND ${name}_test_targets runtimes-test-depends-${name} check-runtimes-${name})
+list(APPEND test_targets ${${name}_test_targets})
 foreach(target_name IN LISTS SUB_CHECK_TARGETS)
   set(${target_name}-${name} ${target_name})
   list(APPEND ${name}_test_targets ${target_name}-${name})
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109625: [compiler-rt] Ensure required deps for tests targets are actually built

[Petr Hosek via Phabricator via cfe-commits]

phosek accepted this revision.
phosek added a comment.
This revision is now accepted and ready to land.

LGTM thanks for debugging this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109625

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109625: [compiler-rt] Ensure required deps for tests targets are actually built

[Leonard Chan via Phabricator via cfe-commits]

leonardchan updated this revision to Diff 376042.
leonardchan edited the summary of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109625

Files:
  llvm/runtimes/CMakeLists.txt


Index: llvm/runtimes/CMakeLists.txt
===
--- llvm/runtimes/CMakeLists.txt
+++ llvm/runtimes/CMakeLists.txt
@@ -294,6 +294,7 @@
 set(runtimes-test-depends-${name} runtimes-test-depends)
 set(check-runtimes-${name} check-runtimes)
 list(APPEND ${name}_test_targets runtimes-test-depends-${name} 
check-runtimes-${name})
+list(APPEND test_targets ${${name}_test_targets})
 foreach(target_name IN LISTS SUB_CHECK_TARGETS)
   set(${target_name}-${name} ${target_name})
   list(APPEND ${name}_test_targets ${target_name}-${name})


Index: llvm/runtimes/CMakeLists.txt
===
--- llvm/runtimes/CMakeLists.txt
+++ llvm/runtimes/CMakeLists.txt
@@ -294,6 +294,7 @@
 set(runtimes-test-depends-${name} runtimes-test-depends)
 set(check-runtimes-${name} check-runtimes)
 list(APPEND ${name}_test_targets runtimes-test-depends-${name} check-runtimes-${name})
+list(APPEND test_targets ${${name}_test_targets})
 foreach(target_name IN LISTS SUB_CHECK_TARGETS)
   set(${target_name}-${name} ${target_name})
   list(APPEND ${name}_test_targets ${target_name}-${name})
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109625: [compiler-rt] Ensure required deps for tests targets are actually built

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

In D109625#3029581 , @phosek wrote:

> I'm still unsure if this is the right strategy because it creates a 
> dependency cycle. Specifically, you have the LLVM build trigger the build of 
> runtimes, and that build would invoke LLVM build again to ensure that the 
> necessary tools have been built. That shouldn't be necessary though. The LLVM 
> build already ensures that those tools are being built by making the runtimes 
> build depend on these tools, see 
> https://github.com/llvm/llvm-project/blob/ac2daacb310cbb1732de1c139be7a0e8e982169e/llvm/runtimes/CMakeLists.txt#L461
>
> What might be needed is a way to tell the compiler-rt build where to find 
> these tools. I looked elsewhere in LLVM and it seems like Polly is already 
> taking that approach, see 
> https://github.com/llvm/llvm-project/blob/main/polly/test/CMakeLists.txt#L6. 
> Do you think that we could use this strategy?

Ah, so it seems that both`test_targets` and `SUB_CHECK_TARGETS` in 
https://github.com/llvm/llvm-project/blob/ac2daacb310cbb1732de1c139be7a0e8e982169e/llvm/runtimes/CMakeLists.txt#L474
 are both empty, so these dependencies are never added. Lemme see if I can find 
why they're empty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109625

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109625: [compiler-rt] Ensure required deps for tests targets are actually built

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

I'm still unsure if this is the right strategy because it creates a dependency 
cycle. Specifically, you have the LLVM build trigger the build of runtimes, and 
that build would invoke LLVM build again to ensure that the necessary tools 
have been built. That shouldn't be necessary though. The LLVM build already 
ensures that those tools are being built by making the runtimes build depend on 
these tools, see 
https://github.com/llvm/llvm-project/blob/ac2daacb310cbb1732de1c139be7a0e8e982169e/llvm/runtimes/CMakeLists.txt#L461

What might be needed is a way to tell the compiler-rt build where to find these 
tools. I looked elsewhere in LLVM and it seems like Polly is already taking 
that approach, see 
https://github.com/llvm/llvm-project/blob/main/polly/test/CMakeLists.txt#L6. Do 
you think that we could use this strategy?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109625

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109625: [compiler-rt] Ensure required deps for tests targets are actually built

[Leonard Chan via Phabricator via cfe-commits]

leonardchan updated this revision to Diff 375713.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109625

Files:
  compiler-rt/test/CMakeLists.txt


Index: compiler-rt/test/CMakeLists.txt
===
--- compiler-rt/test/CMakeLists.txt
+++ compiler-rt/test/CMakeLists.txt
@@ -1,5 +1,6 @@
 # Needed for lit support in standalone builds.
 include(AddLLVM)
+include(LLVMExternalProjectUtils)
 
 option(COMPILER_RT_TEST_STANDALONE_BUILD_LIBS
   "When set to ON and testing in a standalone build, test the runtime \
@@ -36,6 +37,21 @@
 if (WIN32)
   list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS KillTheDoctor)
 endif()
+  elseif(COMPILER_RT_STANDALONE_BUILD)
+# These are tools needed for testing in a standalone/runtimes build, but we
+# don't have a corresponding CMAKE_* flag for. These will need to be 
rebuilt.
+set(LIT_ONLY_TOOLS FileCheck count not)
+foreach(dep ${LIT_ONLY_TOOLS})
+  if (NOT TARGET ${dep})
+llvm_ExternalProject_BuildCmd(build_${dep} ${dep} ${BINARY_DIR})
+add_custom_target(${dep}
+  COMMAND ${build_${dep}}
+  WORKING_DIRECTORY ${BINARY_DIR}
+  VERBATIM USES_TERMINAL)
+  endif()
+endforeach()
+
+list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS ${LIT_ONLY_TOOLS})
   endif()
   # Tests use C++ standard library headers.
   if (TARGET cxx-headers OR HAVE_LIBCXX)


Index: compiler-rt/test/CMakeLists.txt
===
--- compiler-rt/test/CMakeLists.txt
+++ compiler-rt/test/CMakeLists.txt
@@ -1,5 +1,6 @@
 # Needed for lit support in standalone builds.
 include(AddLLVM)
+include(LLVMExternalProjectUtils)
 
 option(COMPILER_RT_TEST_STANDALONE_BUILD_LIBS
   "When set to ON and testing in a standalone build, test the runtime \
@@ -36,6 +37,21 @@
 if (WIN32)
   list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS KillTheDoctor)
 endif()
+  elseif(COMPILER_RT_STANDALONE_BUILD)
+# These are tools needed for testing in a standalone/runtimes build, but we
+# don't have a corresponding CMAKE_* flag for. These will need to be rebuilt.
+set(LIT_ONLY_TOOLS FileCheck count not)
+foreach(dep ${LIT_ONLY_TOOLS})
+  if (NOT TARGET ${dep})
+llvm_ExternalProject_BuildCmd(build_${dep} ${dep} ${BINARY_DIR})
+add_custom_target(${dep}
+  COMMAND ${build_${dep}}
+  WORKING_DIRECTORY ${BINARY_DIR}
+  VERBATIM USES_TERMINAL)
+  endif()
+endforeach()
+
+list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS ${LIT_ONLY_TOOLS})
   endif()
   # Tests use C++ standard library headers.
   if (TARGET cxx-headers OR HAVE_LIBCXX)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109625: [compiler-rt] Ensure required deps for tests targets are actually built

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

Updated to exclude readelf. We also don't need any of the other tools added in 
the `NOT COMPILER_RT_STANDALONE_BUILD` case, so I just added them to 
`LIT_ONLY_TOOLS` in the `COMPILER_RT_STANDALONE_BUILD` case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109625

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109625: [compiler-rt] Ensure required deps for tests targets are actually built

[Leonard Chan via Phabricator via cfe-commits]

leonardchan updated this revision to Diff 374984.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109625

Files:
  compiler-rt/cmake/Modules/AddCompilerRT.cmake
  compiler-rt/test/CMakeLists.txt


Index: compiler-rt/test/CMakeLists.txt
===
--- compiler-rt/test/CMakeLists.txt
+++ compiler-rt/test/CMakeLists.txt
@@ -1,5 +1,6 @@
 # Needed for lit support in standalone builds.
 include(AddLLVM)
+include(LLVMExternalProjectUtils)
 
 option(COMPILER_RT_TEST_STANDALONE_BUILD_LIBS
   "When set to ON and testing in a standalone build, test the runtime \
@@ -36,6 +37,21 @@
 if (WIN32)
   list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS KillTheDoctor)
 endif()
+  elseif(COMPILER_RT_STANDALONE_BUILD)
+# These are tools needed for testing in a standalone/runtimes build, but we
+# don't have a corresponding CMAKE_* flag for. These will need to be 
rebuilt.
+set(LIT_ONLY_TOOLS FileCheck count not clang-resource-headers)
+foreach(dep ${LIT_ONLY_TOOLS})
+  if (NOT TARGET ${dep})
+llvm_ExternalProject_BuildCmd(build_${dep} ${dep} ${BINARY_DIR})
+add_custom_target(${dep}
+  COMMAND ${build_${dep}}
+  WORKING_DIRECTORY ${BINARY_DIR}
+  VERBATIM USES_TERMINAL)
+  endif()
+endforeach()
+
+list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS ${LIT_ONLY_TOOLS})
   endif()
   # Tests use C++ standard library headers.
   if (TARGET cxx-headers OR HAVE_LIBCXX)
Index: compiler-rt/cmake/Modules/AddCompilerRT.cmake
===
--- compiler-rt/cmake/Modules/AddCompilerRT.cmake
+++ compiler-rt/cmake/Modules/AddCompilerRT.cmake
@@ -598,6 +598,7 @@
 CMAKE_OBJCOPY
 CMAKE_OBJDUMP
 CMAKE_STRIP
+CMAKE_READELF
 CMAKE_SYSROOT
 LIBCXX_HAS_MUSL_LIBC
 PYTHON_EXECUTABLE


Index: compiler-rt/test/CMakeLists.txt
===
--- compiler-rt/test/CMakeLists.txt
+++ compiler-rt/test/CMakeLists.txt
@@ -1,5 +1,6 @@
 # Needed for lit support in standalone builds.
 include(AddLLVM)
+include(LLVMExternalProjectUtils)
 
 option(COMPILER_RT_TEST_STANDALONE_BUILD_LIBS
   "When set to ON and testing in a standalone build, test the runtime \
@@ -36,6 +37,21 @@
 if (WIN32)
   list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS KillTheDoctor)
 endif()
+  elseif(COMPILER_RT_STANDALONE_BUILD)
+# These are tools needed for testing in a standalone/runtimes build, but we
+# don't have a corresponding CMAKE_* flag for. These will need to be rebuilt.
+set(LIT_ONLY_TOOLS FileCheck count not clang-resource-headers)
+foreach(dep ${LIT_ONLY_TOOLS})
+  if (NOT TARGET ${dep})
+llvm_ExternalProject_BuildCmd(build_${dep} ${dep} ${BINARY_DIR})
+add_custom_target(${dep}
+  COMMAND ${build_${dep}}
+  WORKING_DIRECTORY ${BINARY_DIR}
+  VERBATIM USES_TERMINAL)
+  endif()
+endforeach()
+
+list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS ${LIT_ONLY_TOOLS})
   endif()
   # Tests use C++ standard library headers.
   if (TARGET cxx-headers OR HAVE_LIBCXX)
Index: compiler-rt/cmake/Modules/AddCompilerRT.cmake
===
--- compiler-rt/cmake/Modules/AddCompilerRT.cmake
+++ compiler-rt/cmake/Modules/AddCompilerRT.cmake
@@ -598,6 +598,7 @@
 CMAKE_OBJCOPY
 CMAKE_OBJDUMP
 CMAKE_STRIP
+CMAKE_READELF
 CMAKE_SYSROOT
 LIBCXX_HAS_MUSL_LIBC
 PYTHON_EXECUTABLE
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109625: [compiler-rt] Ensure required deps for tests targets are actually built

[Petr Hosek via Phabricator via cfe-commits]

phosek added inline comments.



Comment at: compiler-rt/test/CMakeLists.txt:48
+  if (NOT TARGET ${dep})
+llvm_ExternalProject_BuildCmd(build_${dep} ${dep} ${BINARY_DIR})
+add_custom_target(${dep}

leonardchan wrote:
> leonardchan wrote:
> > phosek wrote:
> > > This is going to run Ninja in the LLVM build once for each dependency 
> > > listed above, correct? That seems quite expensive.
> > > 
> > > We already pass most of these to the child build via corresponding CMake 
> > > variables, see 
> > > https://github.com/llvm/llvm-project/blob/ed921282e551f2252ccfcbddd7a85ad8a006ed3f/llvm/cmake/modules/LLVMExternalProjectUtils.cmake#L160
> > > 
> > > For example, if just need some readelf implementation and not necessarily 
> > > llvm-readelf, it may be better to use the value of `CMAKE_READELF` and 
> > > propagate that down to tests through substitution (that is wherever the 
> > > tests invoke `llvm-readelf`, we would replace it with `%readelf`).
> > > 
> > > We're still going to need this logic for tools where there's no 
> > > corresponding CMake variable like `FileCheck` but it should be 
> > > significantly fewer ones.
> > So it looks like only FileCheck, count, and not are required but don't have 
> > cmake variables. Would what I have now be ok where instead we just iterate 
> > over a trimmed list of tools?
> I lied, there's also clang-resource-headers and llvm-readelf.
`llvm-readelf` should be addressed by D110313.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109625

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109625: [compiler-rt] Ensure required deps for tests targets are actually built

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.



Comment at: compiler-rt/test/CMakeLists.txt:48
+  if (NOT TARGET ${dep})
+llvm_ExternalProject_BuildCmd(build_${dep} ${dep} ${BINARY_DIR})
+add_custom_target(${dep}

leonardchan wrote:
> phosek wrote:
> > This is going to run Ninja in the LLVM build once for each dependency 
> > listed above, correct? That seems quite expensive.
> > 
> > We already pass most of these to the child build via corresponding CMake 
> > variables, see 
> > https://github.com/llvm/llvm-project/blob/ed921282e551f2252ccfcbddd7a85ad8a006ed3f/llvm/cmake/modules/LLVMExternalProjectUtils.cmake#L160
> > 
> > For example, if just need some readelf implementation and not necessarily 
> > llvm-readelf, it may be better to use the value of `CMAKE_READELF` and 
> > propagate that down to tests through substitution (that is wherever the 
> > tests invoke `llvm-readelf`, we would replace it with `%readelf`).
> > 
> > We're still going to need this logic for tools where there's no 
> > corresponding CMake variable like `FileCheck` but it should be 
> > significantly fewer ones.
> So it looks like only FileCheck, count, and not are required but don't have 
> cmake variables. Would what I have now be ok where instead we just iterate 
> over a trimmed list of tools?
I lied, there's also clang-resource-headers and llvm-readelf.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109625

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109625: [compiler-rt] Ensure required deps for tests targets are actually built

[Leonard Chan via Phabricator via cfe-commits]

leonardchan updated this revision to Diff 374396.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109625

Files:
  compiler-rt/test/CMakeLists.txt


Index: compiler-rt/test/CMakeLists.txt
===
--- compiler-rt/test/CMakeLists.txt
+++ compiler-rt/test/CMakeLists.txt
@@ -1,5 +1,6 @@
 # Needed for lit support in standalone builds.
 include(AddLLVM)
+include(LLVMExternalProjectUtils)
 
 option(COMPILER_RT_TEST_STANDALONE_BUILD_LIBS
   "When set to ON and testing in a standalone build, test the runtime \
@@ -28,14 +29,34 @@
 # When ANDROID, we build tests with the host compiler (i.e. CMAKE_C_COMPILER),
 # and run tests with tools from the host toolchain.
 if(NOT ANDROID)
-  if(NOT COMPILER_RT_STANDALONE_BUILD AND NOT LLVM_RUNTIMES_BUILD)
-# Use LLVM utils and Clang from the same build tree.
-list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS
+  set(LIT_TEST_DEPS
   clang clang-resource-headers FileCheck count not llvm-config llvm-nm 
llvm-objdump
   llvm-readelf llvm-readobj llvm-size llvm-symbolizer compiler-rt-headers 
sancov)
-if (WIN32)
-  list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS KillTheDoctor)
-endif()
+  if (WIN32)
+list(APPEND LIT_TEST_DEPS KillTheDoctor)
+  endif()
+
+  if(NOT COMPILER_RT_STANDALONE_BUILD AND NOT LLVM_RUNTIMES_BUILD)
+# Use LLVM utils and Clang from the same build tree.
+list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS ${LIT_TEST_DEPS})
+  elseif(COMPILER_RT_STANDALONE_BUILD)
+# If we are making a standalone build, we need to ensure some targets used
+# for testing are known and built as deps.
+list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS ${LIT_TEST_DEPS})
+
+# These are tools needed for testing in a standalone/runtimes build, but we
+# don't have a corresponding CMAKE_* flag for. These will need to be 
rebuilt.
+set(LIT_ONLY_TOOLS
+FileCheck count not clang-resource-headers llvm-readelf)
+foreach(dep ${LIT_ONLY_TOOLS})
+  if (NOT TARGET ${dep})
+llvm_ExternalProject_BuildCmd(build_${dep} ${dep} ${BINARY_DIR})
+add_custom_target(${dep}
+  COMMAND ${build_${dep}}
+  WORKING_DIRECTORY ${BINARY_DIR}
+  VERBATIM USES_TERMINAL)
+  endif()
+endforeach()
   endif()
   # Tests use C++ standard library headers.
   if (TARGET cxx-headers OR HAVE_LIBCXX)


Index: compiler-rt/test/CMakeLists.txt
===
--- compiler-rt/test/CMakeLists.txt
+++ compiler-rt/test/CMakeLists.txt
@@ -1,5 +1,6 @@
 # Needed for lit support in standalone builds.
 include(AddLLVM)
+include(LLVMExternalProjectUtils)
 
 option(COMPILER_RT_TEST_STANDALONE_BUILD_LIBS
   "When set to ON and testing in a standalone build, test the runtime \
@@ -28,14 +29,34 @@
 # When ANDROID, we build tests with the host compiler (i.e. CMAKE_C_COMPILER),
 # and run tests with tools from the host toolchain.
 if(NOT ANDROID)
-  if(NOT COMPILER_RT_STANDALONE_BUILD AND NOT LLVM_RUNTIMES_BUILD)
-# Use LLVM utils and Clang from the same build tree.
-list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS
+  set(LIT_TEST_DEPS
   clang clang-resource-headers FileCheck count not llvm-config llvm-nm llvm-objdump
   llvm-readelf llvm-readobj llvm-size llvm-symbolizer compiler-rt-headers sancov)
-if (WIN32)
-  list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS KillTheDoctor)
-endif()
+  if (WIN32)
+list(APPEND LIT_TEST_DEPS KillTheDoctor)
+  endif()
+
+  if(NOT COMPILER_RT_STANDALONE_BUILD AND NOT LLVM_RUNTIMES_BUILD)
+# Use LLVM utils and Clang from the same build tree.
+list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS ${LIT_TEST_DEPS})
+  elseif(COMPILER_RT_STANDALONE_BUILD)
+# If we are making a standalone build, we need to ensure some targets used
+# for testing are known and built as deps.
+list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS ${LIT_TEST_DEPS})
+
+# These are tools needed for testing in a standalone/runtimes build, but we
+# don't have a corresponding CMAKE_* flag for. These will need to be rebuilt.
+set(LIT_ONLY_TOOLS
+FileCheck count not clang-resource-headers llvm-readelf)
+foreach(dep ${LIT_ONLY_TOOLS})
+  if (NOT TARGET ${dep})
+llvm_ExternalProject_BuildCmd(build_${dep} ${dep} ${BINARY_DIR})
+add_custom_target(${dep}
+  COMMAND ${build_${dep}}
+  WORKING_DIRECTORY ${BINARY_DIR}
+  VERBATIM USES_TERMINAL)
+  endif()
+endforeach()
   endif()
   # Tests use C++ standard library headers.
   if (TARGET cxx-headers OR HAVE_LIBCXX)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109625: [compiler-rt] Ensure required deps for tests targets are actually built

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.



Comment at: compiler-rt/test/CMakeLists.txt:48
+  if (NOT TARGET ${dep})
+llvm_ExternalProject_BuildCmd(build_${dep} ${dep} ${BINARY_DIR})
+add_custom_target(${dep}

phosek wrote:
> This is going to run Ninja in the LLVM build once for each dependency listed 
> above, correct? That seems quite expensive.
> 
> We already pass most of these to the child build via corresponding CMake 
> variables, see 
> https://github.com/llvm/llvm-project/blob/ed921282e551f2252ccfcbddd7a85ad8a006ed3f/llvm/cmake/modules/LLVMExternalProjectUtils.cmake#L160
> 
> For example, if just need some readelf implementation and not necessarily 
> llvm-readelf, it may be better to use the value of `CMAKE_READELF` and 
> propagate that down to tests through substitution (that is wherever the tests 
> invoke `llvm-readelf`, we would replace it with `%readelf`).
> 
> We're still going to need this logic for tools where there's no corresponding 
> CMake variable like `FileCheck` but it should be significantly fewer ones.
So it looks like only FileCheck, count, and not are required but don't have 
cmake variables. Would what I have now be ok where instead we just iterate over 
a trimmed list of tools?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109625

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109625: [compiler-rt] Ensure required deps for tests targets are actually built

[Leonard Chan via Phabricator via cfe-commits]

leonardchan updated this revision to Diff 374393.
leonardchan retitled this revision from "[compiler-rt] Ensure LIT_TEST_DEP 
targets are actually built" to "[compiler-rt] Ensure required deps for tests 
targets are actually built".

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109625

Files:
  compiler-rt/test/CMakeLists.txt


Index: compiler-rt/test/CMakeLists.txt
===
--- compiler-rt/test/CMakeLists.txt
+++ compiler-rt/test/CMakeLists.txt
@@ -1,5 +1,6 @@
 # Needed for lit support in standalone builds.
 include(AddLLVM)
+include(LLVMExternalProjectUtils)
 
 option(COMPILER_RT_TEST_STANDALONE_BUILD_LIBS
   "When set to ON and testing in a standalone build, test the runtime \
@@ -28,14 +29,34 @@
 # When ANDROID, we build tests with the host compiler (i.e. CMAKE_C_COMPILER),
 # and run tests with tools from the host toolchain.
 if(NOT ANDROID)
-  if(NOT COMPILER_RT_STANDALONE_BUILD AND NOT LLVM_RUNTIMES_BUILD)
-# Use LLVM utils and Clang from the same build tree.
-list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS
+  set(LIT_TEST_DEPS
   clang clang-resource-headers FileCheck count not llvm-config llvm-nm 
llvm-objdump
   llvm-readelf llvm-readobj llvm-size llvm-symbolizer compiler-rt-headers 
sancov)
-if (WIN32)
-  list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS KillTheDoctor)
-endif()
+  if (WIN32)
+list(APPEND LIT_TEST_DEPS KillTheDoctor)
+  endif()
+
+  if(NOT COMPILER_RT_STANDALONE_BUILD AND NOT LLVM_RUNTIMES_BUILD)
+# Use LLVM utils and Clang from the same build tree.
+list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS ${LIT_TEST_DEPS})
+  elseif(COMPILER_RT_STANDALONE_BUILD)
+# If we are making a standalone build, we need to ensure some targets used
+# for testing are known and built as deps.
+list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS ${LIT_TEST_DEPS})
+
+# These are tools needed for testing in a standalone/runtimes build, but we
+# don't have a corresponding CMAKE_* flag for. These will need to be 
rebuilt.
+set(LIT_ONLY_TOOLS
+FileCheck count not)
+foreach(dep ${LIT_ONLY_TOOLS})
+  if (NOT TARGET ${dep})
+llvm_ExternalProject_BuildCmd(build_${dep} ${dep} ${BINARY_DIR})
+add_custom_target(${dep}
+  COMMAND ${build_${dep}}
+  WORKING_DIRECTORY ${BINARY_DIR}
+  VERBATIM USES_TERMINAL)
+  endif()
+endforeach()
   endif()
   # Tests use C++ standard library headers.
   if (TARGET cxx-headers OR HAVE_LIBCXX)


Index: compiler-rt/test/CMakeLists.txt
===
--- compiler-rt/test/CMakeLists.txt
+++ compiler-rt/test/CMakeLists.txt
@@ -1,5 +1,6 @@
 # Needed for lit support in standalone builds.
 include(AddLLVM)
+include(LLVMExternalProjectUtils)
 
 option(COMPILER_RT_TEST_STANDALONE_BUILD_LIBS
   "When set to ON and testing in a standalone build, test the runtime \
@@ -28,14 +29,34 @@
 # When ANDROID, we build tests with the host compiler (i.e. CMAKE_C_COMPILER),
 # and run tests with tools from the host toolchain.
 if(NOT ANDROID)
-  if(NOT COMPILER_RT_STANDALONE_BUILD AND NOT LLVM_RUNTIMES_BUILD)
-# Use LLVM utils and Clang from the same build tree.
-list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS
+  set(LIT_TEST_DEPS
   clang clang-resource-headers FileCheck count not llvm-config llvm-nm llvm-objdump
   llvm-readelf llvm-readobj llvm-size llvm-symbolizer compiler-rt-headers sancov)
-if (WIN32)
-  list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS KillTheDoctor)
-endif()
+  if (WIN32)
+list(APPEND LIT_TEST_DEPS KillTheDoctor)
+  endif()
+
+  if(NOT COMPILER_RT_STANDALONE_BUILD AND NOT LLVM_RUNTIMES_BUILD)
+# Use LLVM utils and Clang from the same build tree.
+list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS ${LIT_TEST_DEPS})
+  elseif(COMPILER_RT_STANDALONE_BUILD)
+# If we are making a standalone build, we need to ensure some targets used
+# for testing are known and built as deps.
+list(APPEND SANITIZER_COMMON_LIT_TEST_DEPS ${LIT_TEST_DEPS})
+
+# These are tools needed for testing in a standalone/runtimes build, but we
+# don't have a corresponding CMAKE_* flag for. These will need to be rebuilt.
+set(LIT_ONLY_TOOLS
+FileCheck count not)
+foreach(dep ${LIT_ONLY_TOOLS})
+  if (NOT TARGET ${dep})
+llvm_ExternalProject_BuildCmd(build_${dep} ${dep} ${BINARY_DIR})
+add_custom_target(${dep}
+  COMMAND ${build_${dep}}
+  WORKING_DIRECTORY ${BINARY_DIR}
+  VERBATIM USES_TERMINAL)
+  endif()
+endforeach()
   endif()
   # Tests use C++ standard library headers.
   if (TARGET cxx-headers OR HAVE_LIBCXX)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits