[PATCH] D154987: [lit] Implement PYTHON directive and config.prologue

Tue Jul 25 12:23:59 PDT 2023

jdenny added a comment.

In D154987#4531915 <https://reviews.llvm.org/D154987#4531915>, @sammccall wrote:

> I'm expecting we'll end up with some way of writing assertions directly within python, rather than just calling out to shell commands that verify themselves.
> (If this isn't added deliberately, then I'd expect it'll turn up anyway via `raise` or `lit.run("false")` or something)

In this patch's current implementation, python compile failures (similar to RUN line shell parse errors) produce "unresolved" tests, and python execution failures (similar to RUN line non-zero exit status) produce "failed" tests.  Yes, test authors could use `raise` or some `lit.run` fallacy to induce test failures.  I don't see why that's bad if the check they want to perform is easier to write in python.

> I think gradual migration is a recipe for getting stuck in a halfway state.

We could migrate lit and all llvm upstream test suites at the same time.  However, I think it would be better to give people time to adjust and make sure there are no surprises.  Moreover, I don't think the halfway state in this case is so dire.  My hope is that PYTHON will stop the development of new lit scripting language features so we don't find ourselves with more features to maintain and possibly migrate away from.  Then we would gradually remove the few that have already landed unless we find a good reason to keep them.

> The difficulty in extending lit cleanly is a good reason for an alternative, and I think python is a good choice. (just not embedded in lit)

So, as @jhenderson suggested, I think this is the issue we should focus discussion on right now.  That is, why embed python (using PYTHON) rather than just call a separate python script?  Why embed multiple shell commands (using RUN) rather than just call a separate shell script?  In my opinion, test authors should do what makes their tests easiest to understand while avoiding repeated code.  Sometimes that means making test code visible directly in the individual test file, and sometimes that means encapsulating it in another file.

>>> It's possible to process lit shell tests with alternate implementations (without python!), I know of at least two...
>>
>> I don't think I've heard about this.  Can you explain more?
>
> Google uses a custom lit test runner to run LLVM tests as part of CI.

Thanks for explaining that.

> This is downstream and LLVM doesn't have any obligation to support any of it, but the ability to be integrated into different environments is a useful LLVM feature, and having clear interfaces for infra allows it to evolve in ways other than bolting on more features.

I think it's too much to ask upstream lit developers to avoid adding new features indefinitely because it causes trouble for downstream users.

> Realistically, none of the standard tools for editing python code will ever work if the code is prefixed with `PYTHON: ` lines and lives in `*.cpp` files, though.

I agree that tools that support standard python would require extensions or shims.  I don't have experience with python tools to know how hard that is.  Then again, people might find it's really not worthwhile.  Complex python will likely end up in a python module that is simply called from a PYTHON line.  That would look a lot like what people already do with `%python` in RUN.  But PYTHON gives people a convenient, well integrated way to keep their python local to the test when that makes the test easier to read and maintain.

> I do like the trick where a single lit test file provides multiple kinds of inputs (e.g. to lit + clang + FileCheck), but it is hard to reason about, and would not like to see it used when the test contains control flow.

Why is control flow the cutoff?

> So readability & tools are a bit better as python-in-lit, and the need for tools isn't so strong.

I'm not sure I'm following.  Which approach does "python-in-lit" refer to here?

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

https://reviews.llvm.org/D154987