From 26b01798f66b5187cd4572e4641774ad97515a35 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Mon, 25 Feb 2019 16:23:46 -0800 Subject: [PATCH] try defining algebraic Executables in the native backend to compose more readable toolchains (#6855) ### Problem As can be seen in `native_toolchain.py` in e.g. #6800, it is often difficult to follow changes to the native backend, especially changes which modify the order of resources such as library and include directories for our linkers and compilers. This is because we have been patching together collections of these resources "by hand", without applying any higher-level structure (explicitly constructing each `path_entries` and `library_dirs` field for every executable, every time, for example). This was done to avoid creating abstractions that might break down due to the rapidly evolving code. We can now take the step of more clearly defining the relationships between the toolchains we construct hierarchically. ### Solution - Add an `ExtensibleAlgebraic` mixin which allows declaring list fields which can be immutably modified one at a time with `prepend_field` and `append_field`, or all at once with `sequence`. - Add a `for_compiler` method to `BaseLinker` to wrap the specific process required to prep our linker for a specific compiler. - Apply all of the above in `native_toolchain.py`. ### Result The compilers and linkers provided by `@rule`s in `native_toolchain.py` are composed with consistent verbs from `ExtensibleAlgebraic`, leading to increased readability. --- .../backend/native/config/environment.py | 177 ++++++++++++---- .../native/subsystems/binaries/binutils.py | 5 +- .../backend/native/subsystems/binaries/gcc.py | 4 +- .../native/subsystems/binaries/llvm.py | 4 +- .../native/subsystems/native_toolchain.py | 192 +++++++----------- .../native/subsystems/xcode_cli_tools.py | 9 +- .../native/tasks/link_shared_libraries.py | 2 +- .../backend/native/tasks/native_compile.py | 11 +- .../subsystems/test_native_toolchain.py | 12 +- 9 files changed, 241 insertions(+), 175 deletions(-) diff --git a/src/python/pants/backend/native/config/environment.py b/src/python/pants/backend/native/config/environment.py index eb6d26c8194..b433535668f 100644 --- a/src/python/pants/backend/native/config/environment.py +++ b/src/python/pants/backend/native/config/environment.py @@ -5,9 +5,10 @@ from __future__ import absolute_import, division, print_function, unicode_literals import os -from abc import abstractproperty +from abc import abstractmethod, abstractproperty from pants.engine.rules import SingletonRule +from pants.util.memo import memoized_classproperty from pants.util.meta import AbstractClass from pants.util.objects import datatype, enum from pants.util.osutil import all_normalized_os_names, get_normalized_os_name @@ -19,9 +20,108 @@ class Platform(enum('normalized_os_name', all_normalized_os_names())): default_value = get_normalized_os_name() -class Executable(AbstractClass): +def _list_field(func): + """A decorator for methods corresponding to list-valued fields of an `ExtensibleAlgebraic`. - @abstractproperty + The result is also wrapped in `abstractproperty`. + """ + wrapped = abstractproperty(func) + wrapped._field_type = 'list' + return wrapped + + +def _algebraic_data(metaclass): + """A class decorator to pull out `_list_fields` from a mixin class for use with a `datatype`.""" + def wrapper(cls): + cls.__bases__ += (metaclass,) + cls._list_fields = metaclass._list_fields + return cls + return wrapper + + +# NB: prototypal inheritance seems *deeply* linked with the idea here! +# TODO: since we are calling these methods from other files, we should remove the leading underscore +# and add testing! +class _ExtensibleAlgebraic(AbstractClass): + """A mixin to make it more concise to coalesce datatypes with related collection fields.""" + + @memoized_classproperty + def _list_fields(cls): + all_list_fields = [] + for field_name in cls.__abstractmethods__: + f = getattr(cls, field_name) + if getattr(f, '_field_type', None) == 'list': + all_list_fields.append(field_name) + return frozenset(all_list_fields) + + @abstractmethod + def copy(self, **kwargs): + """Implementations should have the same behavior as a `datatype()`'s `copy()` method.""" + + class AlgebraicDataError(Exception): pass + + def _single_list_field_operation(self, field_name, list_value, prepend=True): + if field_name not in self._list_fields: + raise self.AlgebraicDataError( + "Field '{}' is not in this object's set of declared list fields: {} (this object is : {})." + .format(field_name, self._list_fields, self)) + cur_value = getattr(self, field_name) + + if prepend: + new_value = list_value + cur_value + else: + new_value = cur_value + list_value + + arg_dict = {field_name: new_value} + return self.copy(**arg_dict) + + def prepend_field(self, field_name, list_value): + """Return a copy of this object with `list_value` prepended to the field named `field_name`.""" + return self._single_list_field_operation(field_name, list_value, prepend=True) + + def append_field(self, field_name, list_value): + """Return a copy of this object with `list_value` appended to the field named `field_name`.""" + return self._single_list_field_operation(field_name, list_value, prepend=False) + + def sequence(self, other, exclude_list_fields=None): + """Return a copy of this object which combines all the fields common to both `self` and `other`. + + List fields will be concatenated. + + The return type of this method is the type of `self` (or whatever `.copy()` returns), but the + `other` argument can be any `_ExtensibleAlgebraic` instance. + """ + exclude_list_fields = frozenset(exclude_list_fields or []) + overwrite_kwargs = {} + + nonexistent_excluded_fields = exclude_list_fields - self._list_fields + if nonexistent_excluded_fields: + raise self.AlgebraicDataError( + "Fields {} to exclude from a sequence() were not found in this object's list fields: {}. " + "This object is {}, the other object is {}." + .format(nonexistent_excluded_fields, self._list_fields, self, other)) + + shared_list_fields = (self._list_fields + & other._list_fields + - exclude_list_fields) + if not shared_list_fields: + raise self.AlgebraicDataError( + "Objects to sequence have no shared fields after excluding {}. " + "This object is {}, with list fields: {}. " + "The other object is {}, with list fields: {}." + .format(exclude_list_fields, self, self._list_fields, other, other._list_fields)) + + for list_field_name in shared_list_fields: + lhs_value = getattr(self, list_field_name) + rhs_value = getattr(other, list_field_name) + overwrite_kwargs[list_field_name] = lhs_value + rhs_value + + return self.copy(**overwrite_kwargs) + + +class _Executable(_ExtensibleAlgebraic): + + @_list_field def path_entries(self): """A list of directory paths containing this executable, to be used in a subprocess's PATH. @@ -37,32 +137,33 @@ def exe_filename(self): :rtype: str """ - # TODO: rename this to 'runtime_library_dirs'! - @abstractproperty - def library_dirs(self): + @_list_field + def runtime_library_dirs(self): """Directories containing shared libraries that must be on the runtime library search path. - Note: this is for libraries needed for the current Executable to run -- see LinkerMixin below + Note: this is for libraries needed for the current _Executable to run -- see _LinkerMixin below for libraries that are needed at link time. - :rtype: list of str """ - @property + @_list_field def extra_args(self): - """Additional arguments used when invoking this Executable. + """Additional arguments used when invoking this _Executable. These are typically placed before the invocation-specific command line arguments. :rtype: list of str """ - return [] _platform = Platform.create() @property - def as_invocation_environment_dict(self): - """A dict to use as this Executable's execution environment. + def invocation_environment_dict(self): + """A dict to use as this _Executable's execution environment. + + This isn't made into an "algebraic" field because its contents (the keys of the dict) are + generally known to the specific class which is overriding this property. Implementations of this + property can then make use of the data in the algebraic fields to populate this dict. :rtype: dict of string -> string """ @@ -72,28 +173,29 @@ def as_invocation_environment_dict(self): }) return { 'PATH': create_path_env_var(self.path_entries), - lib_env_var: create_path_env_var(self.library_dirs), + lib_env_var: create_path_env_var(self.runtime_library_dirs), } +@_algebraic_data(_Executable) class Assembler(datatype([ 'path_entries', 'exe_filename', - 'library_dirs', -]), Executable): - pass + 'runtime_library_dirs', + 'extra_args', +])): pass -class LinkerMixin(Executable): +class _LinkerMixin(_Executable): - @abstractproperty + @_list_field def linking_library_dirs(self): """Directories to search for libraries needed at link time. :rtype: list of str """ - @abstractproperty + @_list_field def extra_object_files(self): """A list of object files required to perform a successful link. @@ -103,8 +205,8 @@ def extra_object_files(self): """ @property - def as_invocation_environment_dict(self): - ret = super(LinkerMixin, self).as_invocation_environment_dict.copy() + def invocation_environment_dict(self): + ret = super(_LinkerMixin, self).invocation_environment_dict.copy() full_library_path_dirs = self.linking_library_dirs + [ os.path.dirname(f) for f in self.extra_object_files @@ -118,19 +220,20 @@ def as_invocation_environment_dict(self): return ret +@_algebraic_data(_LinkerMixin) class Linker(datatype([ 'path_entries', 'exe_filename', - 'library_dirs', + 'runtime_library_dirs', 'linking_library_dirs', 'extra_args', 'extra_object_files', -]), LinkerMixin): pass +])): pass -class CompilerMixin(Executable): +class _CompilerMixin(_Executable): - @abstractproperty + @_list_field def include_dirs(self): """Directories to search for header files to #include during compilation. @@ -138,8 +241,8 @@ def include_dirs(self): """ @property - def as_invocation_environment_dict(self): - ret = super(CompilerMixin, self).as_invocation_environment_dict.copy() + def invocation_environment_dict(self): + ret = super(_CompilerMixin, self).invocation_environment_dict.copy() if self.include_dirs: ret['CPATH'] = create_path_env_var(self.include_dirs) @@ -147,34 +250,36 @@ def as_invocation_environment_dict(self): return ret +@_algebraic_data(_CompilerMixin) class CCompiler(datatype([ 'path_entries', 'exe_filename', - 'library_dirs', + 'runtime_library_dirs', 'include_dirs', 'extra_args', -]), CompilerMixin): +])): @property - def as_invocation_environment_dict(self): - ret = super(CCompiler, self).as_invocation_environment_dict.copy() + def invocation_environment_dict(self): + ret = super(CCompiler, self).invocation_environment_dict.copy() ret['CC'] = self.exe_filename return ret +@_algebraic_data(_CompilerMixin) class CppCompiler(datatype([ 'path_entries', 'exe_filename', - 'library_dirs', + 'runtime_library_dirs', 'include_dirs', 'extra_args', -]), CompilerMixin): +])): @property - def as_invocation_environment_dict(self): - ret = super(CppCompiler, self).as_invocation_environment_dict.copy() + def invocation_environment_dict(self): + ret = super(CppCompiler, self).invocation_environment_dict.copy() ret['CXX'] = self.exe_filename diff --git a/src/python/pants/backend/native/subsystems/binaries/binutils.py b/src/python/pants/backend/native/subsystems/binaries/binutils.py index 69c50463001..d8b3375b0c4 100644 --- a/src/python/pants/backend/native/subsystems/binaries/binutils.py +++ b/src/python/pants/backend/native/subsystems/binaries/binutils.py @@ -24,13 +24,14 @@ def assembler(self): return Assembler( path_entries=self.path_entries(), exe_filename='as', - library_dirs=[]) + runtime_library_dirs=[], + extra_args=[]) def linker(self): return Linker( path_entries=self.path_entries(), exe_filename='ld', - library_dirs=[], + runtime_library_dirs=[], linking_library_dirs=[], extra_args=[], extra_object_files=[], diff --git a/src/python/pants/backend/native/subsystems/binaries/gcc.py b/src/python/pants/backend/native/subsystems/binaries/gcc.py index b61bdfcd498..b0696375d79 100644 --- a/src/python/pants/backend/native/subsystems/binaries/gcc.py +++ b/src/python/pants/backend/native/subsystems/binaries/gcc.py @@ -65,7 +65,7 @@ def c_compiler(self, platform): return CCompiler( path_entries=self.path_entries, exe_filename='gcc', - library_dirs=self._common_lib_dirs(platform), + runtime_library_dirs=self._common_lib_dirs(platform), include_dirs=self._common_include_dirs, extra_args=[]) @@ -91,7 +91,7 @@ def cpp_compiler(self, platform): return CppCompiler( path_entries=self.path_entries, exe_filename='g++', - library_dirs=self._common_lib_dirs(platform), + runtime_library_dirs=self._common_lib_dirs(platform), include_dirs=(self._common_include_dirs + self._cpp_include_dirs), extra_args=[]) diff --git a/src/python/pants/backend/native/subsystems/binaries/llvm.py b/src/python/pants/backend/native/subsystems/binaries/llvm.py index 413d0bd8bd3..9786e5c3990 100644 --- a/src/python/pants/backend/native/subsystems/binaries/llvm.py +++ b/src/python/pants/backend/native/subsystems/binaries/llvm.py @@ -105,7 +105,7 @@ def c_compiler(self): return CCompiler( path_entries=self.path_entries, exe_filename='clang', - library_dirs=self._common_lib_dirs, + runtime_library_dirs=self._common_lib_dirs, include_dirs=self._common_include_dirs, extra_args=[]) @@ -117,7 +117,7 @@ def cpp_compiler(self): return CppCompiler( path_entries=self.path_entries, exe_filename='clang++', - library_dirs=self._common_lib_dirs, + runtime_library_dirs=self._common_lib_dirs, include_dirs=(self._cpp_include_dirs + self._common_include_dirs), extra_args=[]) diff --git a/src/python/pants/backend/native/subsystems/native_toolchain.py b/src/python/pants/backend/native/subsystems/native_toolchain.py index dffa151825f..1885c68e0d9 100644 --- a/src/python/pants/backend/native/subsystems/native_toolchain.py +++ b/src/python/pants/backend/native/subsystems/native_toolchain.py @@ -4,6 +4,8 @@ from __future__ import absolute_import, division, print_function, unicode_literals +from builtins import object + from pants.backend.native.config.environment import (Assembler, CCompiler, CppCompiler, CppToolchain, CToolchain, Linker, Platform) from pants.backend.native.subsystems.binaries.binutils import Binutils @@ -67,10 +69,21 @@ def _libc_dev(self): class LibcObjects(datatype(['crti_object_paths'])): pass -class GCCLinker(datatype([('linker', Linker)])): pass +class LinkerWrapperMixin(object): + + def for_compiler(self, compiler, platform): + """Return a Linker object which is intended to be compatible with the given `compiler`.""" + return (self.linker + # TODO(#6143): describe why the compiler needs to be first on the PATH! + .sequence(compiler, exclude_list_fields=['extra_args', 'path_entries']) + .prepend_field('path_entries', compiler.path_entries) + .copy(exe_filename=compiler.exe_filename)) + + +class GCCLinker(datatype([('linker', Linker)]), LinkerWrapperMixin): pass -class LLVMLinker(datatype([('linker', Linker)])): pass +class LLVMLinker(datatype([('linker', Linker)]), LinkerWrapperMixin): pass class GCCCToolchain(datatype([('c_toolchain', CToolchain)])): pass @@ -128,8 +141,7 @@ def select_gcc_linker(native_toolchain): base_linker = yield Get(BaseLinker, NativeToolchain, native_toolchain) linker = base_linker.linker libc_objects = yield Get(LibcObjects, NativeToolchain, native_toolchain) - linker_with_libc = linker.copy( - extra_object_files=(linker.extra_object_files + libc_objects.crti_object_paths)) + linker_with_libc = linker.append_field('extra_object_files', libc_objects.crti_object_paths) yield GCCLinker(linker_with_libc) @@ -160,36 +172,24 @@ def select_gcc_install_location(gcc): def select_llvm_c_toolchain(platform, native_toolchain): provided_clang = yield Get(CCompiler, LLVM, native_toolchain._llvm) - # These arguments are shared across platforms. - llvm_c_compiler_args = [ - '-x', 'c', '-std=c11', - ] - if platform.normalized_os_name == 'darwin': xcode_clang = yield Get(CCompiler, XCodeCLITools, native_toolchain._xcode_cli_tools) - working_c_compiler = provided_clang.copy( - path_entries=(provided_clang.path_entries + xcode_clang.path_entries), - library_dirs=(provided_clang.library_dirs + xcode_clang.library_dirs), - include_dirs=(provided_clang.include_dirs + xcode_clang.include_dirs), - extra_args=(provided_clang.extra_args + llvm_c_compiler_args + xcode_clang.extra_args)) + joined_c_compiler = provided_clang.sequence(xcode_clang) else: gcc_install = yield Get(GCCInstallLocationForLLVM, GCC, native_toolchain._gcc) provided_gcc = yield Get(CCompiler, GCC, native_toolchain._gcc) - working_c_compiler = provided_clang.copy( - # We need g++'s version of the GLIBCXX library to be able to run, unfortunately. - library_dirs=(provided_gcc.library_dirs + provided_clang.library_dirs), - include_dirs=provided_gcc.include_dirs, - extra_args=(llvm_c_compiler_args + provided_clang.extra_args + gcc_install.as_clang_argv)) + joined_c_compiler = (provided_clang + .sequence(provided_gcc) + .append_field('extra_args', gcc_install.as_clang_argv) + # We need g++'s version of the GLIBCXX library to be able to run. + .prepend_field('runtime_library_dirs', provided_gcc.runtime_library_dirs)) - llvm_linker_wrapper = yield Get(LLVMLinker, NativeToolchain, native_toolchain) - llvm_linker = llvm_linker_wrapper.linker + working_c_compiler = joined_c_compiler.prepend_field('extra_args', [ + '-x', 'c', '-std=c11', + ]) - # TODO(#6855): introduce a more concise way to express these compositions of executables. - working_linker = llvm_linker.copy( - path_entries=(llvm_linker.path_entries + working_c_compiler.path_entries), - exe_filename=working_c_compiler.exe_filename, - library_dirs=(llvm_linker.library_dirs + working_c_compiler.library_dirs), - ) + llvm_linker_wrapper = yield Get(LLVMLinker, NativeToolchain, native_toolchain) + working_linker = llvm_linker_wrapper.for_compiler(working_c_compiler, platform) yield LLVMCToolchain(CToolchain(working_c_compiler, working_linker)) @@ -198,52 +198,42 @@ def select_llvm_c_toolchain(platform, native_toolchain): def select_llvm_cpp_toolchain(platform, native_toolchain): provided_clangpp = yield Get(CppCompiler, LLVM, native_toolchain._llvm) - # These arguments are shared across platforms. - llvm_cpp_compiler_args = [ - '-x', 'c++', '-std=c++11', - # This flag is intended to avoid using any of the headers from our LLVM distribution's C++ - # stdlib implementation, or any from the host system, and instead, use include dirs from the - # XCodeCLITools or GCC. - # TODO(#6143): Determine precisely what this flag does and why it's necessary. - '-nostdinc++', - ] - + # On OSX, we use the libc++ (LLVM) C++ standard library implementation. This is feature-complete + # for OSX, but not for Linux (see https://libcxx.llvm.org/ for more info). if platform.normalized_os_name == 'darwin': - xcode_clangpp = yield Get(CppCompiler, XCodeCLITools, native_toolchain._xcode_cli_tools) - working_cpp_compiler = provided_clangpp.copy( - path_entries=(provided_clangpp.path_entries + xcode_clangpp.path_entries), - library_dirs=(provided_clangpp.library_dirs + xcode_clangpp.library_dirs), - include_dirs=(provided_clangpp.include_dirs + xcode_clangpp.include_dirs), - # On OSX, this uses the libc++ (LLVM) C++ standard library implementation. This is - # feature-complete for OSX, but not for Linux (see https://libcxx.llvm.org/ for more info). - extra_args=(llvm_cpp_compiler_args + provided_clangpp.extra_args + xcode_clangpp.extra_args)) - extra_linking_library_dirs = [] + xcode_clang = yield Get(CppCompiler, XCodeCLITools, native_toolchain._xcode_cli_tools) + joined_cpp_compiler = provided_clangpp.sequence(xcode_clang) + extra_llvm_linking_library_dirs = [] linker_extra_args = [] else: gcc_install = yield Get(GCCInstallLocationForLLVM, GCC, native_toolchain._gcc) provided_gpp = yield Get(CppCompiler, GCC, native_toolchain._gcc) - working_cpp_compiler = provided_clangpp.copy( - # We need g++'s version of the GLIBCXX library to be able to run, unfortunately. - library_dirs=(provided_gpp.library_dirs + provided_clangpp.library_dirs), - # NB: we use g++'s headers on Linux, and therefore their C++ standard library. - include_dirs=provided_gpp.include_dirs, - extra_args=(llvm_cpp_compiler_args + provided_clangpp.extra_args + gcc_install.as_clang_argv)) - # TODO(#6855): why are these necessary? this is very mysterious. - extra_linking_library_dirs = provided_gpp.library_dirs + provided_clangpp.library_dirs + joined_cpp_compiler = (provided_clangpp + .sequence(provided_gpp) + # NB: we use g++'s headers on Linux, and therefore their C++ standard + # library. + .copy(include_dirs=provided_gpp.include_dirs) + .append_field('extra_args', gcc_install.as_clang_argv) + # We need g++'s version of the GLIBCXX library to be able to run. + .prepend_field('runtime_library_dirs', provided_gpp.runtime_library_dirs)) + extra_llvm_linking_library_dirs = provided_gpp.runtime_library_dirs + provided_clangpp.runtime_library_dirs # Ensure we use libstdc++, provided by g++, during the linking stage. linker_extra_args=['-stdlib=libstdc++'] - llvm_linker_wrapper = yield Get(LLVMLinker, NativeToolchain, native_toolchain) - llvm_linker = llvm_linker_wrapper.linker + working_cpp_compiler = joined_cpp_compiler.prepend_field('extra_args', [ + '-x', 'c++', '-std=c++11', + # This flag is intended to avoid using any of the headers from our LLVM distribution's C++ + # stdlib implementation, or any from the host system, and instead, use include dirs from the + # XCodeCLITools or GCC. + # TODO(#6143): Determine precisely what this flag does and why it's necessary. + '-nostdinc++', + ]) - working_linker = llvm_linker.copy( - path_entries=(llvm_linker.path_entries + working_cpp_compiler.path_entries), - exe_filename=working_cpp_compiler.exe_filename, - library_dirs=(llvm_linker.library_dirs + working_cpp_compiler.library_dirs), - linking_library_dirs=(llvm_linker.linking_library_dirs + - extra_linking_library_dirs), - extra_args=(llvm_linker.extra_args + linker_extra_args), - ) + llvm_linker_wrapper = yield Get(LLVMLinker, NativeToolchain, native_toolchain) + working_linker = (llvm_linker_wrapper + .for_compiler(working_cpp_compiler, platform) + .append_field('linking_library_dirs', extra_llvm_linking_library_dirs) + .prepend_field('extra_args', linker_extra_args)) yield LLVMCppToolchain(CppToolchain(working_cpp_compiler, working_linker)) @@ -252,35 +242,23 @@ def select_llvm_cpp_toolchain(platform, native_toolchain): def select_gcc_c_toolchain(platform, native_toolchain): provided_gcc = yield Get(CCompiler, GCC, native_toolchain._gcc) - # GCC needs an assembler, so we provide that (platform-specific) tool here. - assembler = yield Get(Assembler, NativeToolchain, native_toolchain) - - gcc_c_compiler_args = [ - '-x', 'c', '-std=c11', - ] - if platform.normalized_os_name == 'darwin': # GCC needs access to some headers that are only provided by the XCode toolchain # currently (e.g. "_stdio.h"). These headers are unlikely to change across versions, so this is # probably safe. xcode_clang = yield Get(CCompiler, XCodeCLITools, native_toolchain._xcode_cli_tools) - new_include_dirs = provided_gcc.include_dirs + xcode_clang.include_dirs + joined_c_compiler = provided_gcc.sequence(xcode_clang) else: - new_include_dirs = provided_gcc.include_dirs + joined_c_compiler = provided_gcc - working_c_compiler = provided_gcc.copy( - path_entries=(provided_gcc.path_entries + assembler.path_entries), - include_dirs=new_include_dirs, - extra_args=gcc_c_compiler_args) + # GCC needs an assembler, so we provide that (platform-specific) tool here. + assembler = yield Get(Assembler, NativeToolchain, native_toolchain) + working_c_compiler = joined_c_compiler.sequence(assembler).prepend_field('extra_args', [ + '-x', 'c', '-std=c11', + ]) gcc_linker_wrapper = yield Get(GCCLinker, NativeToolchain, native_toolchain) - gcc_linker = gcc_linker_wrapper.linker - - working_linker = gcc_linker.copy( - path_entries=(working_c_compiler.path_entries + gcc_linker.path_entries), - exe_filename=working_c_compiler.exe_filename, - library_dirs=(gcc_linker.library_dirs + working_c_compiler.library_dirs), - ) + working_linker = gcc_linker_wrapper.for_compiler(working_c_compiler, platform) yield GCCCToolchain(CToolchain(working_c_compiler, working_linker)) @@ -289,18 +267,6 @@ def select_gcc_c_toolchain(platform, native_toolchain): def select_gcc_cpp_toolchain(platform, native_toolchain): provided_gpp = yield Get(CppCompiler, GCC, native_toolchain._gcc) - # GCC needs an assembler, so we provide that (platform-specific) tool here. - assembler = yield Get(Assembler, NativeToolchain, native_toolchain) - - gcc_cpp_compiler_args = [ - '-x', 'c++', '-std=c++11', - # This flag is intended to avoid using any of the headers from our LLVM distribution's C++ - # stdlib implementation, or any from the host system, and instead, use include dirs from the - # XCodeCLITools or GCC. - # TODO(#6143): Determine precisely what this flag does and why it's necessary. - '-nostdinc++', - ] - if platform.normalized_os_name == 'darwin': # GCC needs access to some headers that are only provided by the XCode toolchain # currently (e.g. "_stdio.h"). These headers are unlikely to change across versions, so this is @@ -308,29 +274,23 @@ def select_gcc_cpp_toolchain(platform, native_toolchain): # TODO: we should be providing all of these (so we can eventually phase out XCodeCLITools # entirely). xcode_clangpp = yield Get(CppCompiler, XCodeCLITools, native_toolchain._xcode_cli_tools) - working_cpp_compiler = provided_gpp.copy( - path_entries=(provided_gpp.path_entries + assembler.path_entries), - include_dirs=(provided_gpp.include_dirs + xcode_clangpp.include_dirs), - extra_args=(gcc_cpp_compiler_args + provided_gpp.extra_args + xcode_clangpp.extra_args), - ) - extra_linking_library_dirs = [] + joined_cpp_compiler = provided_gpp.sequence(xcode_clangpp) else: - provided_clangpp = yield Get(CppCompiler, LLVM, native_toolchain._llvm) - working_cpp_compiler = provided_gpp.copy( - path_entries=(provided_gpp.path_entries + assembler.path_entries), - extra_args=(gcc_cpp_compiler_args + provided_gpp.extra_args), - ) - extra_linking_library_dirs = provided_gpp.library_dirs + provided_clangpp.library_dirs + joined_cpp_compiler = provided_gpp + + # GCC needs an assembler, so we provide that (platform-specific) tool here. + assembler = yield Get(Assembler, NativeToolchain, native_toolchain) + working_cpp_compiler = joined_cpp_compiler.sequence(assembler).prepend_field('extra_args', [ + '-x', 'c++', '-std=c++11', + # This flag is intended to avoid using any of the headers from our LLVM distribution's C++ + # stdlib implementation, or any from the host system, and instead, use include dirs from the + # XCodeCLITools or GCC. + # TODO(#6143): Determine precisely what this flag does and why it's necessary. + '-nostdinc++', + ]) gcc_linker_wrapper = yield Get(GCCLinker, NativeToolchain, native_toolchain) - gcc_linker = gcc_linker_wrapper.linker - - working_linker = gcc_linker.copy( - path_entries=(working_cpp_compiler.path_entries + gcc_linker.path_entries), - exe_filename=working_cpp_compiler.exe_filename, - library_dirs=(gcc_linker.library_dirs + working_cpp_compiler.library_dirs), - linking_library_dirs=(gcc_linker.linking_library_dirs + extra_linking_library_dirs), - ) + working_linker = gcc_linker_wrapper.for_compiler(working_cpp_compiler, platform) yield GCCCppToolchain(CppToolchain(working_cpp_compiler, working_linker)) diff --git a/src/python/pants/backend/native/subsystems/xcode_cli_tools.py b/src/python/pants/backend/native/subsystems/xcode_cli_tools.py index 4ea8fceaf53..3c2e472785c 100644 --- a/src/python/pants/backend/native/subsystems/xcode_cli_tools.py +++ b/src/python/pants/backend/native/subsystems/xcode_cli_tools.py @@ -134,14 +134,15 @@ def assembler(self): return Assembler( path_entries=self.path_entries(), exe_filename='as', - library_dirs=[]) + runtime_library_dirs=[], + extra_args=[]) @memoized_method def linker(self): return Linker( path_entries=self.path_entries(), exe_filename='ld', - library_dirs=[], + runtime_library_dirs=[], linking_library_dirs=[], extra_args=[MIN_OSX_VERSION_ARG], extra_object_files=[], @@ -152,7 +153,7 @@ def c_compiler(self): return CCompiler( path_entries=self.path_entries(), exe_filename='clang', - library_dirs=self.lib_dirs(), + runtime_library_dirs=self.lib_dirs(), include_dirs=self.include_dirs(), extra_args=[MIN_OSX_VERSION_ARG]) @@ -161,7 +162,7 @@ def cpp_compiler(self): return CppCompiler( path_entries=self.path_entries(), exe_filename='clang++', - library_dirs=self.lib_dirs(), + runtime_library_dirs=self.lib_dirs(), include_dirs=self.include_dirs(include_cpp_inc=True), extra_args=[MIN_OSX_VERSION_ARG]) diff --git a/src/python/pants/backend/native/tasks/link_shared_libraries.py b/src/python/pants/backend/native/tasks/link_shared_libraries.py index f49c31bb2f1..913fa9f334a 100644 --- a/src/python/pants/backend/native/tasks/link_shared_libraries.py +++ b/src/python/pants/backend/native/tasks/link_shared_libraries.py @@ -171,7 +171,7 @@ def _execute_link_request(self, link_request): self.context.log.info("selected linker exe name: '{}'".format(linker.exe_filename)) self.context.log.debug("linker argv: {}".format(cmd)) - env = linker.as_invocation_environment_dict + env = linker.invocation_environment_dict self.context.log.debug("linker invocation environment: {}".format(env)) with self.context.new_workunit(name='link-shared-libraries', diff --git a/src/python/pants/backend/native/tasks/native_compile.py b/src/python/pants/backend/native/tasks/native_compile.py index 8533b9a9567..1c313e2e3ca 100644 --- a/src/python/pants/backend/native/tasks/native_compile.py +++ b/src/python/pants/backend/native/tasks/native_compile.py @@ -8,19 +8,18 @@ from abc import abstractmethod from collections import defaultdict -from pants.backend.native.config.environment import Executable from pants.backend.native.tasks.native_task import NativeTask from pants.base.build_environment import get_buildroot from pants.base.exceptions import TaskError from pants.base.workunit import WorkUnit, WorkUnitLabel from pants.util.memo import memoized_method, memoized_property from pants.util.meta import AbstractClass, classproperty -from pants.util.objects import SubclassesOf, datatype +from pants.util.objects import datatype from pants.util.process_handler import subprocess class NativeCompileRequest(datatype([ - ('compiler', SubclassesOf(Executable)), + 'compiler', # TODO: add type checking for Collection.of()! 'include_dirs', 'sources', @@ -134,11 +133,11 @@ def _compile_settings(self): @abstractmethod def get_compiler(self, native_library_target): - """An instance of `Executable` which can be invoked to compile files. + """An instance of `_CompilerMixin` which can be invoked to compile files. NB: Subclasses will be queried for the compiler instance once and the result cached. - :return: :class:`pants.backend.native.config.environment.Executable` + :return: :class:`pants.backend.native.config.environment._CompilerMixin` """ def _compiler(self, native_library_target): @@ -229,7 +228,7 @@ def _compile(self, compile_request): compiler = compile_request.compiler output_dir = compile_request.output_dir - env = compiler.as_invocation_environment_dict + env = compiler.invocation_environment_dict with self.context.new_workunit( name=self.workunit_label, labels=[WorkUnitLabel.COMPILER]) as workunit: diff --git a/tests/python/pants_test/backend/native/subsystems/test_native_toolchain.py b/tests/python/pants_test/backend/native/subsystems/test_native_toolchain.py index 3dcda4e72cf..d3f822fb042 100644 --- a/tests/python/pants_test/backend/native/subsystems/test_native_toolchain.py +++ b/tests/python/pants_test/backend/native/subsystems/test_native_toolchain.py @@ -56,7 +56,7 @@ def test_gcc_version(self): gcc = gcc_c_toolchain.c_toolchain.c_compiler gcc_version_out = self._invoke_capturing_output( [gcc.exe_filename, '--version'], - env=gcc.as_invocation_environment_dict) + env=gcc.invocation_environment_dict) gcc_version_regex = re.compile('^gcc.*{}$'.format(re.escape(self.gcc_version)), flags=re.MULTILINE) @@ -70,7 +70,7 @@ def test_gpp_version(self): gpp = gcc_cpp_toolchain.cpp_toolchain.cpp_compiler gpp_version_out = self._invoke_capturing_output( [gpp.exe_filename, '--version'], - env=gpp.as_invocation_environment_dict) + env=gpp.invocation_environment_dict) gpp_version_regex = re.compile(r'^g\+\+.*{}$'.format(re.escape(self.gcc_version)), flags=re.MULTILINE) @@ -84,7 +84,7 @@ def test_clang_version(self): clang = llvm_c_toolchain.c_toolchain.c_compiler clang_version_out = self._invoke_capturing_output( [clang.exe_filename, '--version'], - env=clang.as_invocation_environment_dict) + env=clang.invocation_environment_dict) clang_version_regex = re.compile('^clang version {}'.format(re.escape(self.llvm_version)), flags=re.MULTILINE) @@ -100,7 +100,7 @@ def test_clangpp_version(self): clangpp = llvm_cpp_toolchain.cpp_toolchain.cpp_compiler clanggpp_version_out = self._invoke_capturing_output( [clangpp.exe_filename, '--version'], - env=clangpp.as_invocation_environment_dict) + env=clangpp.invocation_environment_dict) self.assertIsNotNone(clangpp_version_regex.search(clanggpp_version_out)) @@ -120,7 +120,7 @@ def _hello_world_source_environment(self, toolchain_type, file_name, contents): def _invoke_compiler(self, compiler, args): cmd = [compiler.exe_filename] + compiler.extra_args + args - env = compiler.as_invocation_environment_dict + env = compiler.invocation_environment_dict # TODO: add an `extra_args`-like field to `Executable`s which allows for overriding env vars # like this, but declaratively! env['LC_ALL'] = 'C' @@ -130,7 +130,7 @@ def _invoke_linker(self, linker, args): cmd = [linker.exe_filename] + linker.extra_args + args return self._invoke_capturing_output( cmd, - linker.as_invocation_environment_dict) + linker.invocation_environment_dict) def _invoke_capturing_output(self, cmd, env=None): env = env or {}