[PATCH] D107162: [lit] Have REQUIRES support the target triple

Wed Aug 4 15:09:16 PDT 2021

probinson added a comment.

In D107162#2926617 <https://reviews.llvm.org/D107162#2926617>, @jdenny wrote:

> @probinson: Can you confirm my understanding, as follows?
>
> - Without this patch, REQUIRES doesn't match the target triple at all (unless it happens to have been added as a feature to a particular test suite).  This patch enables matching the entire target triple or a substring of it.

Correct.

I'll note that there are a bunch of triple-based (and sometimes triple-like) feature keywords added by lit, based on the target triple (see the block starting `if target_triple:` at circa line 106 in llvm/utils/lit/lit/llvm/config.py), which of course could be replaced by regexes in UNSUPPORTED/XFAIL already, and also in REQUIRES if this patch goes in.

> - In any hypothetical test suite, this patch can only expand the number of tests that run because it enables REQUIRES to be more easily satisfied (by the current target triple).  It is impossible for this patch to reduce the number of tests that run.

Correct.

> - In practice so far, only one test is affected.

Out of the projects I generally build: llvm;clang;clang-tools-extra;lld;lldb.  I can try doing "all" projects and see what happens, although my track record on getting clean builds and tests is imperfect.

> A few comments, assuming my above understanding is correct:
>
> - Hiding test failures accidentally is bad.  Exposing test failures accidentally is not so bad.
> - From that perspective, it seems exactly backward that UNSUPPORTED and XFAIL currently match more broadly than REQUIRES.  Why was that decision made?

"It was like that when I found it."  Others who were more involved in lit in the past might be better able to explain.  Roaming through the history, UNSUPPORTED was added by Eric Fiselier in 2014; it did not support triple-checking at first either, that was added by Peter Collingbourne about a year later.  I didn't try to research the XFAIL history.

> - The existence of a test case and documentation pointing out the existing behavior means this behavior was intentional.  That makes me concerned I've misunderstood what's happening.

I accept that REQUIRES intentionally has not supported target-triple matching; I still think we're better off with it in place, unless someone can come up with a really nice statement of why it's beneficial enough that we should live with the inconsistency and the need for miscellaneous ad-hoc triple substitutes as mentioned above.

> - I do agree that eventually it would be nice to get rid of substring matching in favor of regexes for XFAIL and UNSUPPORTED, where matching more broadly than intended is dangerous.  I'm not sure I buy that argument for REQUIRES.
> - If this patch will be landed, it should update the docs: https://llvm.org/docs/TestingGuide.html#constraining-test-execution

Ah, thanks for pointing to the doc; I will update it.  I see that REQUIRES was not documented at all until 2016 but that's no excuse for not keeping it up-to-date now.

================
Comment at: llvm/utils/lit/tests/Inputs/xunit-output/lit.cfg:10
 config.test_exec_root = None
-config.target_triple = None
+config.target_triple = ''
----------------
jdenny wrote:
> Why does this need to change?
Because the test execution goes through the REQUIRES path, and the target_triple check gets a Python error if it's `None` (says something like "not iterable").  

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

https://reviews.llvm.org/D107162