This is an archive of the discontinued LLVM Phabricator instance.

Include libcxx and libcxxabi tests in check-all
AbandonedPublic

Authored by ldionne on Sep 2 2020, 1:40 PM.

Details

Reviewers
azharudd
Group Reviewers
Restricted Project
Restricted Project
Summary

If these projects are enabled, we should be able to run their tests as part of
check-all itself and not have to invoke them separately. This was the behavior
before, but that changed with 6f69318c72. This re-adds that logic while retaining
the other changes in that commit.

add_lit_testsuite is a wrapper around add_lit_target which allows it to be
included in the check-all too.

Diff Detail

Event Timeline

azharudd created this revision.Sep 2 2020, 1:40 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 2 2020, 1:40 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
azharudd requested review of this revision.Sep 2 2020, 1:40 PM
azharudd edited the summary of this revision. (Show Details)
azharudd edited the summary of this revision. (Show Details)Sep 2 2020, 1:45 PM
ldionne requested changes to this revision.Sep 2 2020, 3:08 PM

As discussed offline, the issue is that add_lit_testsuite does set_property(GLOBAL APPEND PROPERTY LLVM_LIT_PARAMS ${ARG_PARAMS}), which means that all test suites (e.g. libcxxabi and libunwind) will now share the same Lit invocation parameters. However, if those have been configured differently, like if libcxx wants to run with --param enable_exceptions=False but libcxxabi wants to run with --param enable_exceptions=True, that won't be respected. There will be a single top-level Lit invocation that will get passed both --param enable_exceptions=False and --param enable_exceptions=True, and whichever appears last (I think) will be used.

The problem really is that we make one global Lit invocation with check-all. Instead, one approach might be to add_dependencies(check-all check-cxx) when the check-all target exists, but that isn't free of issues either.

This revision now requires changes to proceed.Sep 2 2020, 3:08 PM

We ended up doing that to fix the runtimes build in https://reviews.llvm.org/D97913. I'll be closing this since it's already been applied, but thank for bringing this issue to my attention!

ldionne commandeered this revision.Jul 7 2021, 6:46 AM
ldionne abandoned this revision.
ldionne edited reviewers, added: azharudd; removed: ldionne.