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

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

jdenny added a comment.

In D154987#4501813 <https://reviews.llvm.org/D154987#4501813>, @Endill wrote:

> Thank you for coming up with the idea, and then implementing it!
> Unfortunately, I can't review your implementation from lit standpoint, but it looks solid judging by documentation.

Thanks for reviewing and for the helpful conversations that led here.



================
Comment at: llvm/docs/TestingGuide.rst:317-318
+commands in a single test file (:program:`lit` parallelism is only across test
+files), this approach can produce very slow tests. Unless full coverage is
+critical, it might be better to specify only a cross-section of such a problem
+space. Python functions can be useful for this purpose. For example, we might
----------------
Endill wrote:
> I think we can provide an example for this case as well:
> ```
> from itertools import product
> 
> stds = (98, 11, 14, 17, 20, 23)
> triples = (
>   'x86_64-apple-darwin10.6.0',
>   'x86_64-unknown-linux-gnu'
> )
> for std, triple in product(stds, triples):
>   lit.run(f'%clang_cc1 -std=c++{std} -triple {triple} %s')
> ```
> But it could be better to hold it until we come up with a variant of `run` that runs in parallel with other runs, in order to avoid the issue you're describing.
Agreed.  Let's wait until we have a better handle on such cases.


================
Comment at: llvm/docs/TestingGuide.rst:377
+
+    ; PYTHON: exampleModule.lit = lit # exampleModule.py has: lit = None
+
----------------
Endill wrote:
> I suggest to provide complete example, because description is somewhat difficult to follow, especially for people not too well-versed in Python.
Thanks for that feedback.  Please let me know if the new version is still confusing.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:2243-2244
+
+        # TODO: In the future, extend the 'lit' API to be able to *write*
+        # substitutions?
+
----------------
Endill wrote:
> I don't see why we shouldn't provide such an API to PYTHON directives, but it's out of scope of this patch.
I agree we should consider that later.

One reason is we need to think about the interface.   (For example, I'm currently inclined to have separate define and redefine functions for safety, but others might disagree.)

Another reason is that, if PYTHON directives can change substitutions, it would not generally be possible to expand substitutions in RUN lines appearing after PYTHON directives until those PYTHON directives have been executed. That might impact the "Script:" discussion under D154984, so let's settle that discussion first.


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

https://reviews.llvm.org/D154987