[PATCH] D70660: Add initial tests for update_{llc,cc}_test_checks.py

[Nico Weber via Phabricator via llvm-commits]

thakis added inline comments.


================
Comment at: llvm/test/tools/UpdateTestChecks/lit.local.cfg:49
+    config.substitutions.append(('%python3', shell_quote(py3_exe)))
+    config.available_features.add('python3')
+
----------------
arichardson wrote:
> thakis wrote:
> > Nothing in this patch seems to use this as far as I can tell? Do you need this?
> Yes indeed it is not needed anymore. In an initial version of the patch I used %python3 to invoke `update_cc_test_checks.py` Does this cause issues for you?
> If so feel free to delete.
I finally had time to look into why this is failing on one of my machines. If python3 isn't on the path, then the update_cc_test_checks substitution isn't getting added, and `RUN: %update_cc_test_checks` in the test tries to literally run `%update_cc_test_checks` (which of course fails).

So a possible fix is to add `REQUIRES: python3`to the test.

It seems a bit strange to me to only run some tests if py3 is around, though. Environment-dependent tests like this make analyzing failures more difficult. What stops these tests from running with Py3?

(ps: It'd be nice if add_update_script_substition would expect that the name already contains a leading % instead of adding it. That makes it much easier to grep for `%update_cc_test_checks` and see where it's supposed to be set.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70660