[PATCH] D73980: [lit][compiler-rt] add multi-cfgd test suite discovery

[Yuanfang Chen via Phabricator via llvm-commits]

ychen marked 9 inline comments as done.
ychen added inline comments.


================
Comment at: llvm/docs/CommandGuide/lit.rst:65
+ :file:`{NAME}.site.multi.cfg` when searching for test suites, instead of
+ :file:`lit.cfg`, :file:`lit.site.cfg` and :file:`{NAME}.site.multi.cfg`.
 
----------------
delcypher wrote:
> Is the second
> 
>   :file:`{NAME}.site.multi.cfg`
> 
> supposed to be
> 
>   :file:`lit.site.multi.cfg`
> 
> ?
Yes.


================
Comment at: llvm/utils/lit/CMakeLists.txt:11
 
+# Test multi-configured test suite discovery.
+file(GENERATE OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/lit.site.multi.cfg
----------------
delcypher wrote:
> This might not work on Windows due to the slashes.
> 
> I don't like that we're doing something special here just for this test. This means that the test doesn't even exist if someone tries to run the tests in the source tree.
> Can't we have the multicfg format support relative paths so
> 
> * that additional copying is unnecessary
> * The test gets executed if lit is tested in the source tree.
- Added relative path support
- Removed the code here: the test works like others in the lit/test directory


================
Comment at: llvm/utils/lit/lit/discovery.py:241
+            assert not sub_ts.is_multi_cfg, 'multi cfg only happen at exec side where sub suite is not possible'
+            if sub_ts.configs[0] in ts.configs:
+                continue
----------------
delcypher wrote:
> Why is this here? It deserves a comment at the very least.
I should have removed this. It was intended to address the same issue the above code is addressing now. It could've been removed because when `if sub_ts.configs[0] in ts.configs` is true, `if sub_ts is ts` would also be true during the recursive call of `getTestsInSuite` (line 248). 

```
        if sub_ts is ts:
            continue
```


================
Comment at: llvm/utils/lit/lit/reports.py:13
 
-def by_suite_and_test_path(test):
+def by_suite_and_config_and_test_path(test):
     # Suite names are not necessarily unique.  Include object identity in sort
----------------
delcypher wrote:
> This change is an API break. Is anyone using the old `by_suite_and_test_path` from their lit configuration files?
It seems no in-tree/lnt/test-suite/zorg clients are using this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73980