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

Dan Albert danalbert at google.com
Mon Dec 15 11:38:16 PST 2014


================
Comment at: CMakeLists.txt:93
@@ +92,3 @@
+set(LIBCXX_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
+set(LIBCXX_BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR})
+set(LIBCXX_LIBRARY_DIR ${CMAKE_BINARY_DIR})
----------------
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?

================
Comment at: CMakeLists.txt:94
@@ +93,3 @@
+set(LIBCXX_BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR})
+set(LIBCXX_LIBRARY_DIR ${CMAKE_BINARY_DIR})
+
----------------
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).

================
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:
----------------
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?

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

================
Comment at: test/lit.cfg:589
@@ +588,3 @@
+        lit_config.load_config(config, site_cfg)
+        raise SystemExit
+
----------------
You're actually raising a class here, not an exception. This would catch on `except type as ex` rather than `except SystemExit as ex` :)

http://reviews.llvm.org/D6255

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the cfe-commits mailing list