refactor XCodeCLITools's method of finding files

src/python/pants/backend/native/config/environment.py

@@ -11,7 +11,7 @@
 from pants.engine.rules import SingletonRule
 from pants.util.objects import datatype
 from pants.util.osutil import all_normalized_os_names, get_normalized_os_name
-from pants.util.strutil import create_path_env_var
+from pants.util.strutil import create_path_env_var, safe_shlex_join
 
 
 class Platform(datatype(['normalized_os_name'])):
@@ -78,18 +78,30 @@ class Assembler(datatype([
   pass
 
 
+# FIXME: rename this to SharedLibLinker or something!
 class Linker(datatype([
     'path_entries',
     'exe_filename',
     'library_dirs',
 ]), Executable):
 
+  # FIXME(???): we need a way to compose executables hygienically -- this will work because we use
+  # safe shlex methods, but we should really be composing each datatype's members, and only
+  # creating an environment at the very end. This could be done declaratively -- something like:
+  # { 'LIBRARY_PATH': DelimitedPathDirectoryEnvVar(...) }.
+  # We could also just use safe_shlex_join() and create_path_env_var() and keep all the state in the
+  # environment -- but then we have to remember to use those each time we specialize.
   def get_invocation_environment_dict(self, platform):
     ret = super(Linker, self).get_invocation_environment_dict(platform).copy()
 
+    all_ldflags_for_platform = platform.resolve_platform_specific({
+      'darwin': lambda: ['-mmacosx-version-min=10.11', '-Wl,-dylib'],
+      'linux': lambda: ['-shared'],
+    })
     ret.update({
       'LDSHARED': self.exe_filename,
       'LIBRARY_PATH': create_path_env_var(self.library_dirs),
+      'LDFLAGS': safe_shlex_join(all_ldflags_for_platform),
     })
 
     return ret
@@ -109,6 +121,12 @@ def get_invocation_environment_dict(self, platform):
     if self.include_dirs:
       ret['CPATH'] = create_path_env_var(self.include_dirs)
 
+    all_cflags_for_platform = platform.resolve_platform_specific({
+      'darwin': lambda: ['-mmacosx-version-min=10.11'],
+      'linux': lambda: [],
+    })
+    ret['CFLAGS'] = safe_shlex_join(all_cflags_for_platform)
+
     return ret
 
 
@@ -118,8 +136,13 @@ class CCompiler(datatype([
     'library_dirs',
     'include_dirs',
 ]), CompilerMixin):
-  # TODO: set CC in this datatype's env dict, and CXX in CppCompiler!
-  pass
+
+  def get_invocation_environment_dict(self, platform):
+    ret = super(CCompiler, self).get_invocation_environment_dict(platform).copy()
+
+    ret['CC'] = self.exe_filename
+
+    return ret
 
 
 class CppCompiler(datatype([
@@ -128,7 +151,13 @@ class CppCompiler(datatype([
     'library_dirs',
     'include_dirs',
 ]), CompilerMixin):
-  pass
+
+  def get_invocation_environment_dict(self, platform):
+    ret = super(CppCompiler, self).get_invocation_environment_dict(platform).copy()
+
+    ret['CXX'] = self.exe_filename
+
+    return ret
 
 
 # FIXME: make this an @rule, after we can automatically produce LibcDev (see #5788).

src/python/pants/backend/native/subsystems/binaries/llvm.py

@@ -100,7 +100,8 @@ def c_compiler(self, platform):
     return CCompiler(
       path_entries=path_entries,
       exe_filename=exe_filename,
-      library_dirs=lib_search_dirs)
+      library_dirs=lib_search_dirs,
+      include_dirs=[])
 
   def cpp_compiler(self, platform):
     exe_filename = 'clang++'
@@ -113,7 +114,8 @@ def cpp_compiler(self, platform):
     return CppCompiler(
       path_entries=path_entries,
       exe_filename=exe_filename,
-      library_dirs=lib_search_dirs)
+      library_dirs=lib_search_dirs,
+      include_dirs=[])
 
 
 @rule(Linker, [Select(Platform), Select(LLVM)])

src/python/pants/backend/native/subsystems/native_toolchain.py

@@ -90,6 +90,7 @@ def select_linker(platform, native_toolchain):
   # or a linker (e.g. when we compile native code from `python_dist()`s), use the environment from
   # the linker object (in addition to any further customizations), which has the paths from the C
   # and C++ compilers baked in.
+  # FIXME(???): we need a way to compose executables hygienically.
   linker = Linker(
     path_entries=(
       c_compiler.path_entries +
@@ -107,6 +108,8 @@ def select_linker(platform, native_toolchain):
 
 @rule(CCompiler, [Select(Platform), Select(NativeToolchain)])
 def select_c_compiler(platform, native_toolchain):
+  # FIXME(???): we should be using our clang, not the provided one, in all cases, and just using
+  # XCodeCLITools to get the lib and include dirs we need from the host.
   if platform.normalized_os_name == 'darwin':
     c_compiler = yield Get(CCompiler, XCodeCLITools, native_toolchain._xcode_cli_tools)
   else:

src/python/pants/backend/native/subsystems/xcode_cli_tools.py

@@ -10,9 +10,7 @@
 from pants.backend.native.config.environment import Assembler, CCompiler, CppCompiler, Linker
 from pants.engine.rules import RootRule, rule
 from pants.engine.selectors import Select
-from pants.option.custom_types import dir_option
 from pants.subsystem.subsystem import Subsystem
-from pants.util.dirutil import is_executable
 from pants.util.memo import memoized_method, memoized_property
 
 
@@ -26,7 +24,7 @@ class XCodeCLITools(Subsystem):
 
   options_scope = 'xcode-cli-tools'
 
-  REQUIRED_FILES_DEFAULT = {
+  REQUIRED_FILES = {
     'bin': [
       'as',
       'cc',
@@ -36,19 +34,23 @@ class XCodeCLITools(Subsystem):
       'ld',
       'lipo',
     ],
-    'include': ['_stdio.h'],
+    'include': [
+      '_stdio.h',
+    ],
     'lib': [],
   }
 
-  _OSX_BASE_PREFIX = '/usr'
-  _XCODE_TOOLCHAIN_BASE = '/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr'
-
-  # This comes from running clang -###.
-  INCLUDE_SEARCH_DIRS_DEFAULT = [
-    os.path.join(_OSX_BASE_PREFIX, 'local/include'),
-    os.path.join(_XCODE_TOOLCHAIN_BASE, 'lib/clang/9.1.0/include'),
-    os.path.join(_XCODE_TOOLCHAIN_BASE, 'include'),
-    os.path.join(_OSX_BASE_PREFIX, 'include'),
+  INSTALL_PREFIXES_DEFAULT = [
+    # Prefer files from this installation directory, if available. This doesn't appear to be
+    # populated with e.g. header files on travis.
+    '/usr',
+    # Populated by the XCode CLI tools.
+    '/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr',
+    # Populated by the XCode app.
+    # TODO: ???/where do all these directories come from?
+    '/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr',
+    '/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/9.1.0',
+    '/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr',
   ]
 
   class XCodeToolsUnavailable(Exception):
@@ -61,102 +63,90 @@ class XCodeToolsInvalid(Exception):
   def register_options(cls, register):
     super(XCodeCLITools, cls).register_options(register)
 
-    register('--install-prefix', type=dir_option, default=cls._OSX_BASE_PREFIX, advanced=True,
-             help='Location where the XCode command-line developer tools have been installed. '
-                  'Under this directory should be at least {} subdirectories.'
-                  .format(cls.REQUIRED_FILES_DEFAULT.keys()))
-
-    register('--required-files', type=dict, default=cls.REQUIRED_FILES_DEFAULT, advanced=True,
-             help='Files that should exist within the XCode CLI tools installation.')
+    register('--install-prefixes', type=list, default=cls.INSTALL_PREFIXES_DEFAULT, advanced=True,
+             help='Locations to search for resources from the XCode CLI tools, including a '
+                  'compiler, linker, header files, and some libraries. '
+                  'Under this directory should be some selection of these subdirectories: {}.'
+                  .format(cls.REQUIRED_FILES.keys()))
 
-    register('--include-search-dirs', type=list, default=cls.INCLUDE_SEARCH_DIRS_DEFAULT,
-             advanced=True,
-             help='Directories to search, in order, for files in #include directives.')
-
-  # TODO: Obtaining options values from a subsystem should be made ergonomic as we move to complete
-  # #5788.
   @memoized_property
-  def _install_prefix(self):
-    return self.get_options().install_prefix
+  def _all_existing_install_prefixes(self):
+    return [pfx for pfx in self.get_options().install_prefixes if os.path.isdir(pfx)]
 
-  @memoized_property
-  def _required_files(self):
-    return self.get_options().required_files
+  # NB: We use @memoized_method in this file for methods which may raise.
+  @memoized_method
+  def _get_existing_subdirs(self, subdir_name):
+    maybe_subdirs = [os.path.join(pfx, subdir_name) for pfx in self._all_existing_install_prefixes]
+    existing_dirs = [existing_dir for existing_dir in maybe_subdirs if os.path.isdir(existing_dir)]
+
+    required_files_for_dir = self.REQUIRED_FILES.get(subdir_name)
+    if required_files_for_dir:
+      for fname in required_files_for_dir:
+        found = False
+        for subdir in existing_dirs:
+          full_path = os.path.join(subdir, fname)
+          if os.path.isfile(full_path):
+            found = True
+            continue
+
+        if not found:
+          raise self.XCodeToolsUnavailable(
+            "File '{fname}' in subdirectory '{subdir_name}' does not exist at any of the specified "
+            "prefixes. This file is required to build native code on this platform. You may need "
+            "to install the XCode command line developer tools from the Mac App Store.\n\n"
+            "If the XCode tools are installed and you are still seeing this message, please file "
+            "an issue at https://github.com/pantsbuild/pants/issues/new describing your "
+            "OSX environment and which file could not be found.\n"
+            "The existing install prefixes were: {pfxs}. These can be extended with "
+            "--{scope}-install-prefixes."
+            .format(fname=fname,
+                    subdir_name=subdir_name,
+                    pfxs=self._all_existing_install_prefixes,
+                    scope=self.get_options_scope_equivalent_flag_component()))
+
+    return existing_dirs
 
-  @memoized_property
-  def _include_search_dirs(self):
-    return self.get_options().include_search_dirs
+  @memoized_method
+  def path_entries(self):
+    return self._get_existing_subdirs('bin')
 
-  @memoized_property
-  def _lib_dir(self):
-    return os.path.join(self._install_prefix, 'lib')
-
-  def _check_executables_exist(self):
-    for subdir, items in self._required_files.items():
-      for fname in items:
-        file_path = os.path.join(self._install_prefix, subdir, fname)
-        if subdir == 'bin':
-          if not is_executable(file_path):
-            raise self.XCodeToolsUnavailable(
-              "'{exe}' is not an executable file, but it is required to build "
-              "native code on this platform. You may need to install the XCode "
-              "command line developer tools from the Mac App Store. "
-              "(with --required-files={req}, --install-prefix={pfx})"
-              .format(exe=file_path, req=self._required_files, pfx=self._install_prefix))
-        else:
-          if not os.path.isfile(file_path):
-            raise self.XCodeToolsUnavailable(
-              "The file at path '{path}' does not exist, but it is required to build "
-              "native code on this platform. You may need to install the XCode "
-              "command line developer tools from the Mac App Store. "
-              "(with --required-files={req}, --install-prefix={pfx})"
-              .format(path=file_path, req=self._required_files, pfx=self._install_prefix))
+  @memoized_method
+  def lib_dirs(self):
+    return self._get_existing_subdirs('lib')
 
   @memoized_method
-  def path_entries(self):
-    self._check_executables_exist()
-    return [os.path.join(self._install_prefix, 'bin')]
-
-  def _verify_tool_name(self, name):
-    # TODO: introduce some more generic way to do this that can be applied to
-    # provided tools as well, and actually checks whether the file exists within
-    # some set of PATH entries and is executable.
-    known_tools = self._required_files['bin']
-    if name in known_tools:
-      return name
-    raise self.XCodeToolsInvalid(
-      "Internal error: {!r} is not a valid tool. Known tools are: {!r}."
-      .format(name, known_tools))
+  def include_dirs(self):
+    return self._get_existing_subdirs('include')
 
   @memoized_method
   def assembler(self):
     return Assembler(
       path_entries=self.path_entries(),
-      exe_filename=self._verify_tool_name('as'),
+      exe_filename='as',
       library_dirs=[])
 
   @memoized_method
   def linker(self):
     return Linker(
       path_entries=self.path_entries(),
-      exe_filename=self._verify_tool_name('ld'),
+      exe_filename='ld',
       library_dirs=[])
 
   @memoized_method
   def c_compiler(self):
     return CCompiler(
       path_entries=self.path_entries(),
-      exe_filename=self._verify_tool_name('clang'),
-      library_dirs=[self._lib_dir],
-      include_dirs=self._include_search_dirs)
+      exe_filename='clang',
+      library_dirs=self.lib_dirs(),
+      include_dirs=self.include_dirs())
 
   @memoized_method
   def cpp_compiler(self):
     return CppCompiler(
       path_entries=self.path_entries(),
-      exe_filename=self._verify_tool_name('clang++'),
-      library_dirs=[self._lib_dir],
-      include_dirs=self._include_search_dirs)
+      exe_filename='clang++',
+      library_dirs=self.lib_dirs(),
+      include_dirs=self.include_dirs())
 
 
 @rule(Assembler, [Select(XCodeCLITools)])

src/python/pants/backend/python/tasks/setup_py.py

@@ -151,11 +151,6 @@ class SetupPyExecutionEnvironment(datatype([
     'setup_py_native_tools',
 ])):
 
-  _SHARED_CMDLINE_ARGS = {
-    'darwin': lambda: ['-mmacosx-version-min=10.11', '-Wl,-dylib'],
-    'linux': lambda: ['-shared'],
-  }
-
   def as_environment(self):
     ret = {}
 
@@ -164,13 +159,22 @@ def as_environment(self):
 
     native_tools = self.setup_py_native_tools
     if native_tools:
-      ret['CC'] = native_tools.c_compiler.exe_filename
-      ret['CXX'] = native_tools.cpp_compiler.exe_filename
-
-      all_ldflags = native_tools.platform.resolve_platform_specific(self._SHARED_CMDLINE_ARGS)
-      ret['LDFLAGS'] = safe_shlex_join(all_ldflags)
-
-      ret.update(native_tools.linker.get_invocation_environment_dict(native_tools.platform))
+      plat = native_tools.platform
+
+      # FIXME: add an as_tuple() method to datatype so we can do this with a for loop!
+      ret.update(native_tools.c_compiler.get_invocation_environment_dict(plat))
+      ret.update(native_tools.cpp_compiler.get_invocation_environment_dict(plat))
+      ret.update(native_tools.linker.get_invocation_environment_dict(plat))
+
+      # FIXME(???): we need a way to compose executables hygienically -- this will work because we
+      # use safe shlex methods, but we should probably be composing each datatype's members, and
+      # only creating an environment at the very end.
+      prev_ldflags = safe_shlex_split(ret.get('LDFLAGS', ''))
+      all_new_ldflags = plat.resolve_platform_specific({
+        'darwin': lambda: prev_ldflags + ['-undefined', 'dynamic_lookup'],
+        'linux': lambda: prev_ldflags,
+      })
+      ret['LDFLAGS'] = safe_shlex_join(all_new_ldflags)
 
     return ret