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

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

@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.
- 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.
- In practice so far, only one test is affected.

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?
- 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 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



================
Comment at: llvm/utils/lit/tests/Inputs/xunit-output/lit.cfg:10
 config.test_exec_root = None
-config.target_triple = None
+config.target_triple = ''
----------------
Why does this need to change?


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

https://reviews.llvm.org/D107162