[PATCH] D37756: [lit] Force site configs to be run before source-tree configs

[David L. Jones via Phabricator via llvm-commits]

dlj added a comment.

This looks like an excellent cleanup.



================
Comment at: clang-tools-extra/test/lit.cfg:89
 # Tweak the PATH to include the tools dir and the scripts dir.
-if clang_tools_binary_dir is not None:
-    clang_tools_dir = getattr(config, 'clang_tools_dir', None)
-    if not clang_tools_dir:
-        lit_config.fatal('No Clang tools dir set!')
-    llvm_tools_dir = getattr(config, 'llvm_tools_dir', None)
-    if not llvm_tools_dir:
-        lit_config.fatal('No LLVM tools dir set!')
-    path = os.path.pathsep.join((
-            clang_tools_dir, llvm_tools_dir, config.environment['PATH']))
-    config.environment['PATH'] = path
-
-    clang_libs_dir = getattr(config, 'clang_libs_dir', None)
-    if not clang_libs_dir:
-        lit_config.fatal('No Clang libs dir set!')
-    llvm_libs_dir = getattr(config, 'llvm_libs_dir', None)
-    if not llvm_libs_dir:
-        lit_config.fatal('No LLVM libs dir set!')
-    path = os.path.pathsep.join((clang_libs_dir, llvm_libs_dir,
-                                 config.environment.get('LD_LIBRARY_PATH','')))
-    config.environment['LD_LIBRARY_PATH'] = path
-
-###
-
-# Check that the object root is known.
-if config.test_exec_root is None:
-    # Otherwise, we haven't loaded the site specific configuration (the user is
-    # probably trying to run on a test file directly, and either the site
-    # configuration hasn't been created by the build system, or we are in an
-    # out-of-tree build situation).
-
-    # Check for 'clang_site_config' user parameter, and use that if available.
-    site_cfg = lit_config.params.get('clang_tools_extra_site_config', None)
-    if site_cfg and os.path.exists(site_cfg):
-        lit_config.load_config(config, site_cfg)
-        raise SystemExit
-
-    # Try to detect the situation where we are using an out-of-tree build by
-    # looking for 'llvm-config'.
-    #
-    # FIXME: I debated (i.e., wrote and threw away) adding logic to
-    # automagically generate the lit.site.cfg if we are in some kind of fresh
-    # build situation. This means knowing how to invoke the build system though,
-    # and I decided it was too much magic. We should solve this by just having
-    # the .cfg files generated during the configuration step.
-
-    llvm_config = lit.util.which('llvm-config', config.environment['PATH'])
-    if not llvm_config:
-        lit_config.fatal('No site specific configuration available!')
-
-    # Get the source and object roots.
-    llvm_src_root = lit.util.capture(['llvm-config', '--src-root']).strip()
-    llvm_obj_root = lit.util.capture(['llvm-config', '--obj-root']).strip()
-    clang_src_root = os.path.join(llvm_src_root, "tools", "clang")
-    clang_obj_root = os.path.join(llvm_obj_root, "tools", "clang")
-
-    clang_tools_extra_src_root = os.path.join(clang_src_root, "tools", "extra")
-    clang_tools_extra_obj_root = os.path.join(clang_obj_root, "tools", "extra")
-    # Validate that we got a tree which points to here, using the standard
-    # tools/clang layout.
-    this_src_root = os.path.dirname(config.test_source_root)
-    if os.path.realpath(clang_tools_extra_src_root) != os.path.realpath(this_src_root):
-        lit_config.fatal('No site specific configuration available!')
-
-    # Check that the site specific configuration exists.
-    site_cfg = os.path.join(clang_tools_extra_obj_root, 'test', 'lit.site.cfg')
-    if not os.path.exists(site_cfg):
-        lit_config.fatal(
-            'No site specific configuration available! You may need to '
-            'run "make test" in your Clang build directory.')
-
-    # Okay, that worked. Notify the user of the automagic, and reconfigure.
-    lit_config.note('using out-of-tree build at %r' % clang_obj_root)
-    lit_config.load_config(config, site_cfg)
-    raise SystemExit
-
-###
+path = os.path.pathsep.join((
+        config.clang_tools_dir, config.llvm_tools_dir, config.environment['PATH']))
----------------
Minor nit: it looks like you can assign this directly instead of using the 'path' intermediate. (This would avoid shadowing imports of os.path.)

Same in other cfg files.


================
Comment at: clang-tools-extra/test/lit.cfg:97
 
 import os
 
----------------
(Another minor nit) This is duplicated at the top, so it should be fine to remove this extra import.


================
Comment at: compiler-rt/test/lit.common.cfg:256
 except OSError:
-  print("Could not find llvm-config in " + llvm_tools_dir)
+  print("Could not find llvm-config in " + config.llvm_tools_dir)
   exit(42)
----------------
Can this use lit_config.fatal? (I suspect so, but I'm not sure whether this is *always* evaluated in a context where lit_config is available.)


https://reviews.llvm.org/D37756