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

[Dan Liew via Phabricator via llvm-commits]

delcypher requested changes to this revision.
delcypher added a comment.
This revision now requires changes to proceed.

I've done another pass.



================
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`.
 
----------------
Is the second

  :file:`{NAME}.site.multi.cfg`

supposed to be

  :file:`lit.site.multi.cfg`

?


================
Comment at: llvm/docs/CommandGuide/lit.rst:254
+  /path/to/cfg_dir1/lit.site.cfg
+  /path/to/cfg_dir2/lit.site.cfg
 
----------------
This format doesn't seem like to it would handle future changes very well. Could this be a more structured format, e.g. JSON?


================
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
----------------
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.


================
Comment at: llvm/utils/lit/lit/discovery.py:62
 
-        # We found a test suite, create a new config for it and load it.
-        if litConfig.debug:
-            litConfig.note('loading suite config %r' % cfgpath)
-
-        cfg = TestingConfig.fromdefaults(litConfig)
-        cfg.load_from_path(cfgpath, litConfig)
-        source_root = os.path.realpath(cfg.test_source_root or path)
-        exec_root = os.path.realpath(cfg.test_exec_root or path)
-        return Test.TestSuite(cfg.name, source_root, exec_root, cfg), ()
+        # Read multi-cfg test suite
+        cfgpaths = []
----------------
Parsing of the file format probably belongs in its own function.


================
Comment at: llvm/utils/lit/lit/discovery.py:126
 
-def getLocalConfig(ts, path_in_suite, litConfig, cache):
+def getLocalConfig(tc, path_in_suite, litConfig, cache):
     def search1(path_in_suite):
----------------
If you're going to rename `ts` perhaps we should use a more helpful name, e.g. `testConfig`.


================
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
----------------
Why is this here? It deserves a comment at the very least.


================
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
----------------
This change is an API break. Is anyone using the old `by_suite_and_test_path` from their lit configuration files?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73980