[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

JamesNagurne added a comment. In D121838#3543421 , @JamesNagurne wrote: > After some investigation, I found that we did not set LLVM_INCLUDE_TESTS from > the top-level project. This seems like an oversight because we build the > test-depends (and check

[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

JamesNagurne added a comment. After some investigation, I found that we did not set LLVM_INCLUDE_TESTS from the top-level project. This seems like an oversight because we build the test-depends (and check-all) regularly. After seeing LLVM_INCLUDE_TESTS to ON, the builds worked. This commit clea

[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

JamesNagurne added a comment. Hi Sam, I've got a downstream build system that is failing here and I'm trying to figure out why. After your commit, I get an error during bootstrapped runtime builds: 05-27 14:41 __main__ INFO [202/796] cd /path/to/builddir/runtimes/runtimes-target-bins

[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG7cc8377f2c57: Generalize "check-all" umbrella targets, use for che

[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

sammccall added a subscriber: ldionne. sammccall added a comment. In D121838#3470478 , @ldionne wrote: > I think @phosek is the right person to look at this. I looked at it and it > seems fine, but I don't know how the runtimes tests are setup well enoug

[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

sammccall updated this revision to Diff 425995. sammccall added a comment. re-trigger CI Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121838/new/ https://reviews.llvm.org/D121838 Files: clang-tools-extra/CMakeLists.txt clang-tools-extra/test/

[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

ldionne resigned from this revision. ldionne added a comment. I think @phosek is the right person to look at this. I looked at it and it seems fine, but I don't know how the runtimes tests are setup well enough to spot an issue if there were one. Nit: I'd suggest rebasing and re-uploading the p

[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

sammccall added 1 blocking reviewer(s): ldionne. sammccall added a comment. The changes to runtimes/ are mostly mechanical, but Herald says I'm supposed to get a review :-) @ldionne, @phosek, looks like this touches the same bits from D121276 , WDYT? Repositor

[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

sammccall added inline comments. Comment at: llvm/cmake/modules/AddLLVM.cmake:1876 +get_property(gather_names GLOBAL PROPERTY LLVM_LIT_UMBRELLAS) +set_property(GLOBAL APPEND PROPERTY LLVM_LIT_UMBRELLAS ${name}) +foreach(name ${gather_names}) hokein wr

[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

sammccall updated this revision to Diff 423648. sammccall marked 5 inline comments as done. sammccall added a comment. Herald added projects: Sanitizers, libc++, libc++abi. Herald added subscribers: libcxx-commits, Sanitizers. Herald added a reviewer: libc++. Herald added a reviewer: libc++abi. Thi

[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

kbobyrev added a comment. LG to me with the comments that Haojian made! Comment at: llvm/cmake/modules/AddLLVM.cmake:1827 +# Convert a target name like check-clangd to a variable name like CLANG. +function(umbrella_lit_testsuite_var target outvar) hokein wrot

[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

hokein added a comment. +1, I think this is an improvement, it looks good overall, some nits. Comment at: clang-tools-extra/CMakeLists.txt:49 +endif() + nit: remove the extra blank line. Comment at: clang/CMakeLists.txt:196 +umbrella_lit_

[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. I'm not an expert in CMake, but the changes look reasonable from what I do know. The current bot failures all look to be unrelated to the changes in this patch as best I can tell

[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

sammccall updated this revision to Diff 421028. sammccall added a comment. rebased, come and get me robots Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121838/new/ https://reviews.llvm.org/D121838 Files: clang-tools-extra/CMakeLists.txt clang

[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

sammccall added a comment. In D121838#3433472 , @tstellar wrote: > Can you elaborate more on the problem this is solving? Also, what are the > user visible changes? Most directly, check-clang-tools does not currently run clangd or clang-pseudo tests, b

[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

tstellar added a comment. Can you elaborate more on the problem this is solving? Also, what are the user visible changes? Will check-all and check-clang run the same set of tests as before? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121838/ne

[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

aaron.ballman added a comment. > In D121838#3433022 , @kbobyrev > wrote: > >> I think this is no longer [WIP] but rather review-ready, right? > > It's both WIP and review-ready :-) > There are some mechanical cleanups left to do as mentioned in the commen