Address most of @danalbert's comments. Changes incoming.
================
Comment at: CMakeLists.txt:94
@@ +93,3 @@
+set(LIBCXX_BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR})
+set(LIBCXX_LIBRARY_DIR ${CMAKE_BINARY_DIR})
+
----------------
danalbert wrote:
> 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).
> 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.
================
Comment at: test/lit.cfg:567
@@ +566,3 @@
+# Infer the test_exec_root from the libcxx_object root.
+libcxx_obj_root = getattr(config, 'libcxx_obj_root', None)
+if libcxx_obj_root is not None:
----------------
danalbert wrote:
> 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?
`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.
================
Comment at: test/lit.cfg:572
@@ +571,3 @@
+# Check that the test exec root is known.
+if config.test_exec_root is None:
+ # Otherwise, we haven't loaded the site specific configuration (the user is
----------------
danalbert wrote:
> 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?
`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
================
Comment at: test/lit.cfg:589
@@ +588,3 @@
+ lit_config.load_config(config, site_cfg)
+ raise SystemExit
+
----------------
danalbert wrote:
> You're actually raising a class here, not an exception. This would catch on
> `except type as ex` rather than `except SystemExit as ex` :)
Thanks! This needs to be fixed in Clang's `lit.cfg` as well.
http://reviews.llvm.org/D6255
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
