[libcxx] Teach libcxx's lit configuration new ways to find lit.site.cfg

[EricWF]

Summary

Currently to run tests in tree you need to symlink the lit.site.cfg file generated by the cmake build into the source tree, and teach your VCS to ignore it.

This allows the user to specify where to find the lit.site.cfg file two different ways:

• lit_site_config lit parameter
• LIT_SITE_CONFIG enviroment variable.

example usage:

lit -sv --param=libcxx_site_config=path/to/libcxx-build/test/lit.site.cfg path/to/tests

Or

export LIBCXX_SITE_CONFIG=path/to/libcxx-build/test/lit.site.cfg
lit -sv path/to/tests

The command line parameter will override the environment variable.
If neither options are present a warning is issued and the lit.cfg file is loaded directly.

[EricWF]

Add error handling for when a bad lit.site.cfg value is found.

[danalbert]

Should this be a feature of LIT proper? $ lit --config path/to/lit.cfg test/dir

The environment variable should stay libc++ specific though.

[EricWF]

In D6255#6, @danalbert wrote:

Should this be a feature of LIT proper? $ lit --config path/to/lit.cfg test/dir

I don't think it can be for a couple of reasons.

First, the way tests are discovered does not seem to allow the config file to be specified manually. I tried implementing it but it just doesn't seem to fit. The reason this works is because the lit.site.cfg file is found naturally through lit's test discovery.

Second, LIT can be invoked on multiple test suites at once. The lit.site.cfg file is specific to a single suite. I don't see how you could specify the configuration file unambiguously. For example:

lit -sv --config path/to/config/lit.site.cfg path/to/libcxx/test/ path/to/clang/test/

I hope that cleared things up.

[jroelofs]

Is this compatible with --config-prefix=?

test/lit.site.cfg

2

We don't usually put the vim part of this in llvm things, do we?

[EricWF]

I actually think that by preloading the LIT test suite cache I can make this a proper LIT feature. Sorry @danalbert, you were right. However I can't keep the environment variable libc++ specific.

[EricWF]

So I have a patch for LIT proper that does something like this. However I'm not sure it fits nicely into LIT. I'll put it up for review and get @ddunbar's opinion of it. It may just be best to handle this on our end.

[danalbert]

test/lit.site.cfg

2

Should be in all of these imo (actually, it should just be a .py, but this will do).

[EricWF]

I think this is the better direction to actually go. clang's lit.cfg does something similar to this but it uses llvm-config to find lit.site.cfg. I don't think changing LIT proper is the right way to go.

Pinging this patch.

[EricWF]

I updated the patch to function almost exactly the way clang's lit.cfg does it.

[danalbert]

CMakeLists.txt

93 ↗

(On Diff #17098)

This one changed from BINARY_DIR to CURRENT_BINARY_DIR. Was that intentional? Why do we need both LIBCXX_BINARY_DIR and LIBCXX_LIBRARY_DIR?

94 ↗

(On Diff #17098)

Ignore the above comment (phabricator keeps locking up whenever I try to delete or edit it).

This one is always joined with '/lib' when it's used. Should just do the join here instead (or move it into lib/CMakeLists.txt).

test/lit.cfg

567 ↗

(On Diff #17098)

This will never be true, right?

If I understand what you've done here, you've inverted the process of loading the LIT configs. Instead of the lit.site.cfg delegating to lit.cfg, lit.cfg now loads lit.site.cfg. That makes more sense to me, but this check is a no-op then (since we don't have the config loaded). Don't you also need to remove the load_config in lit.site.cfg.in?

572 ↗

(On Diff #17098)

Is test_exec_root something you've added? If so, it doesn't need to be a part of the config AFACT. If not, where was it being configured before?

589 ↗

(On Diff #17098)

You're actually raising a class here, not an exception. This would catch on except type as ex rather than except SystemExit as ex :)

[EricWF]

Address most of @danalbert's comments. Changes incoming.

CMakeLists.txt

94 ↗

(On Diff #17098)

This one is always joined with '/lib' when it's used. Should just do the join here instead (or move it into lib/CMakeLists.txt).

Yeah, probably. I'll make that change.

test/lit.cfg

567 ↗

(On Diff #17098)

libcxx_obj_root can be present in config at this point.

The make check-libcxx rule actually loads the lit.site.cfg first. This sets libcxx_obj_root, and we don't need to perform any magic to find the build directory.However if you try and run the tests from the source directory then libcxx_obj_root won't be present, and we need to search for and load the site configuration.

Don't you also need to remove the load_config in lit.site.cfg.in?

No. In both cases we need the lit.site.cfg to load lit.cfg. When lit.cfg is the first file loaded, the lit.site.cfg recursively re-loads lit.cfg and the branch on #572 is skipped.

572 ↗

(On Diff #17098)

test_exec_root and test_source_root are documented members of the config object. They are documented as follows:

• test_source_root The filesystem path to the test suite root. For out-of-dir builds this is the directory that will be scanned for tests.
• test_exec_root For out-of-dir builds, the path to the test suite root inside the object directory. This is where tests will be run and temporary output files placed.

Right now these are unused by our test format, but we should probably set them so we can use other test formats in lit.

http://llvm.org/docs/CommandGuide/lit.html#test-suites

589 ↗

(On Diff #17098)

Thanks! This needs to be fixed in Clang's lit.cfg as well.

[EricWF]

Address most of @danalbert's comments. Changes incoming.

[EricWF]

test/lit.cfg

589 ↗

(On Diff #17098)

Actually I think we are both wrong. raise SystemExit seems to be equivalent to raise SystemExit(). If any arguments are added then lit.TestingConfig.load_from_path will exit before running any of the tests. I'll add the braces for clarity.

https://github.com/llvm-mirror/llvm/blob/master/utils/lit/lit/TestingConfig.py#L99

[EricWF]

Address @danalbert's comments. I also sneaked in the removal of config.python_executable which is entirely unused.

[danalbert]

LGTM.

test/lit.cfg

589 ↗

(On Diff #17098)

Oh, interesting.

test/lit.site.cfg

+# -*- Python -*- vim: set syntax=python tabstop=4 expandtab cc=80:
+
+import os
+
+# Try getting the configuration from the libcxx_site_cfg parameter first
+# Then from the LIBCXX_SITE_CFG enviroment variable.
+cfg_path = lit_config.params.get('libcxx_site_cfg',
+                                 os.environ.get('LIBCXX_SITE_CFG'))
+if cfg_path is not None:
+    lit_config.note('Loading site configuration from: %s' % cfg_path)
+    lit_config.load_config(config, cfg_path)
+else:
+    lit_config.warning('Running lit.cfg without a site configuration')
+    main_cfg = os.path.join(os.path.dirname(__file__), 'lit.cfg')
+    lit_config.load_config(config, main_cfg)

www/index.html

   <ul>
     <li><code>cd path/to/libcxx/libcxx</code></li>
     <li><code>alias lit='python path/to/llvm/utils/lit/lit.py'</code></li>
-    <li><code>ln -s path/to/build/dir/projects/libcxx/test/lit.site.cfg
-        test/lit.site.cfg</code></li>
+    <li><code>export LIBCXX_SITE_CFG=path/to/libcxx-build/test/lit.site.cfg
+        </code></li>
     <li><code>lit -sv test/re/ # or whichever subset of tests you're interested
         in</code></li>
   </ul>