From f5120a27508149f0f1cbed8ad6fb80c51324eba3 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Wed, 17 Oct 2018 19:31:53 -0700 Subject: [PATCH] Add testproject with C/C++ interdependent targets, fix native backend design mistake (#6628) ### Problem *This probably blocks #6486 and #6492.* As @CMLivingston and I realized sometime this week (to my *immense* dismay), we had no tests of native library targets depending on each other. When making these examples, I realized two things: 1. `--strict-deps` doesn't really make sense as two separate options for C and C++ targets which can depend on each other. 2. The implementation of `--strict-deps` in the native backend ("caching" native target interdependencies in a product from the compiler tasks) was absurd and unnecessary. I think I vaguely recall that the fact that `LinkSharedLibraries` previously did not have a subsystem dependency on the `CCompileSettings` or `CppCompileSettings` subsystems may have led to a subtle caching bug in the link task when options in those subsystems were modified, but I don't remember all the details right now. Regardless, that bug would have been fixed with this PR -- see the below: ### Solution - Move dependency calculation into a single `NativeBuildSettings` subsystem which is consumed by compile and link tasks, and use it to calculate the dependencies in both tasks. - Drop the very unnecessary `NativeTargetDependencies` product. - Move a ton of logic into `NativeTask` that really should have been there instead of `NativeCompile`, but couldn't because of the hacked together dependency calculation. - Add a testproject with C and C++ targets depending on each other, and an integration test showing the above all works. ### Result We have a working example of C and C++ targets depending on each other, resulting in idiomatic enough C and C++, which also work extremely effectively in a `python_dist()` target. --- .../subsystems/native_build_settings.py | 32 +++++++ .../native_build_step_settings_base.py | 41 +++++++++ .../subsystems/native_compile_settings.py | 37 -------- .../utils/mirrored_target_option_mixin.py | 35 ++++++++ .../pants/backend/native/tasks/c_compile.py | 19 +--- .../pants/backend/native/tasks/cpp_compile.py | 19 +--- .../native/tasks/link_shared_libraries.py | 51 +++-------- .../backend/native/tasks/native_compile.py | 81 +++++------------ .../pants/backend/native/tasks/native_task.py | 88 +++++++++++++++++++ .../python_distribution/ctypes_interop/BUILD | 23 +++++ .../ctypes_interop/__init__.py | 0 .../ctypes_python_pkg/__init__.py | 0 .../ctypes_python_pkg/ctypes_wrapper.py | 39 ++++++++ .../ctypes_interop/main.py | 13 +++ .../ctypes_interop/setup.py | 17 ++++ .../ctypes_interop/some-math/BUILD | 1 + .../ctypes_interop/some-math/some_math.c | 3 + .../ctypes_interop/some-math/some_math.h | 6 ++ .../ctypes_interop/some-more-math/BUILD | 6 ++ .../some-more-math/some_more_math.cpp | 12 +++ .../some-more-math/some_more_math.hpp | 14 +++ .../ctypes_interop/wrapped-math/BUILD | 10 +++ .../wrapped-math/wrapped_math.c | 5 ++ .../wrapped-math/wrapped_math.h | 6 ++ .../python/tasks/test_ctypes_integration.py | 50 +++++++++-- 25 files changed, 433 insertions(+), 175 deletions(-) create mode 100644 src/python/pants/backend/native/subsystems/native_build_settings.py create mode 100644 src/python/pants/backend/native/subsystems/native_build_step_settings_base.py delete mode 100644 src/python/pants/backend/native/subsystems/native_compile_settings.py create mode 100644 src/python/pants/backend/native/subsystems/utils/mirrored_target_option_mixin.py create mode 100644 testprojects/src/python/python_distribution/ctypes_interop/BUILD create mode 100644 testprojects/src/python/python_distribution/ctypes_interop/__init__.py create mode 100644 testprojects/src/python/python_distribution/ctypes_interop/ctypes_python_pkg/__init__.py create mode 100644 testprojects/src/python/python_distribution/ctypes_interop/ctypes_python_pkg/ctypes_wrapper.py create mode 100644 testprojects/src/python/python_distribution/ctypes_interop/main.py create mode 100644 testprojects/src/python/python_distribution/ctypes_interop/setup.py create mode 100644 testprojects/src/python/python_distribution/ctypes_interop/some-math/BUILD create mode 100644 testprojects/src/python/python_distribution/ctypes_interop/some-math/some_math.c create mode 100644 testprojects/src/python/python_distribution/ctypes_interop/some-math/some_math.h create mode 100644 testprojects/src/python/python_distribution/ctypes_interop/some-more-math/BUILD create mode 100644 testprojects/src/python/python_distribution/ctypes_interop/some-more-math/some_more_math.cpp create mode 100644 testprojects/src/python/python_distribution/ctypes_interop/some-more-math/some_more_math.hpp create mode 100644 testprojects/src/python/python_distribution/ctypes_interop/wrapped-math/BUILD create mode 100644 testprojects/src/python/python_distribution/ctypes_interop/wrapped-math/wrapped_math.c create mode 100644 testprojects/src/python/python_distribution/ctypes_interop/wrapped-math/wrapped_math.h diff --git a/src/python/pants/backend/native/subsystems/native_build_settings.py b/src/python/pants/backend/native/subsystems/native_build_settings.py new file mode 100644 index 00000000000..f3ed2e8cb53 --- /dev/null +++ b/src/python/pants/backend/native/subsystems/native_build_settings.py @@ -0,0 +1,32 @@ +# coding=utf-8 +# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md). +# Licensed under the Apache License, Version 2.0 (see LICENSE). + +from __future__ import absolute_import, division, print_function, unicode_literals + +from pants.backend.native.subsystems.utils.mirrored_target_option_mixin import \ + MirroredTargetOptionMixin +from pants.subsystem.subsystem import Subsystem + + +class NativeBuildSettings(Subsystem, MirroredTargetOptionMixin): + """Any settings relevant to a compiler and/or linker invocation.""" + options_scope = 'native-build-settings' + + mirrored_option_to_kwarg_map = { + 'strict_deps': 'strict_deps', + } + + @classmethod + def register_options(cls, register): + super(NativeBuildSettings, cls).register_options(register) + + # TODO: rename this so it's clear it is not the same option as JVM strict deps! + register('--strict-deps', type=bool, default=True, fingerprint=True, advanced=True, + help="Whether to include only dependencies directly declared in the BUILD file " + "for C and C++ targets by default. If this is False, all transitive dependencies " + "are used when compiling and linking native code. C and C++ targets may override " + "this behavior with the strict_deps keyword argument as well.") + + def get_strict_deps_value_for_target(self, target): + return self.get_target_mirrored_option('strict_deps', target) diff --git a/src/python/pants/backend/native/subsystems/native_build_step_settings_base.py b/src/python/pants/backend/native/subsystems/native_build_step_settings_base.py new file mode 100644 index 00000000000..1c9a8335cef --- /dev/null +++ b/src/python/pants/backend/native/subsystems/native_build_step_settings_base.py @@ -0,0 +1,41 @@ +# coding=utf-8 +# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md). +# Licensed under the Apache License, Version 2.0 (see LICENSE). + +from __future__ import absolute_import, division, print_function, unicode_literals + +from pants.backend.native.subsystems.utils.mirrored_target_option_mixin import \ + MirroredTargetOptionMixin +from pants.subsystem.subsystem import Subsystem + + +class NativeBuildStepSettingsBase(Subsystem, MirroredTargetOptionMixin): + + mirrored_option_to_kwarg_map = { + 'fatal_warnings': 'fatal_warnings', + } + + @classmethod + def register_options(cls, register): + super(NativeBuildStepSettingsBase, cls).register_options(register) + + # TODO: implement compiler_option_sets as an interface to platform/host-specific optimization + # flags! + register('--fatal-warnings', type=bool, default=True, fingerprint=True, advanced=True, + help='The default for the "fatal_warnings" argument for targets of this language.') + + def get_fatal_warnings_value_for_target(self, target): + return self.get_target_mirrored_option('fatal_warnings', target) + + +class CCompileSettings(NativeBuildStepSettingsBase): + options_scope = 'c-compile-settings' + + +class CppCompileSettings(NativeBuildStepSettingsBase): + options_scope = 'cpp-compile-settings' + + +# TODO: add a fatal_warnings kwarg to NativeArtifact and make a LinkSharedLibrariesSettings subclass +# of NativeBuildStepSettingsBase here! The method should work even though NativeArtifact is not a +# Target. diff --git a/src/python/pants/backend/native/subsystems/native_compile_settings.py b/src/python/pants/backend/native/subsystems/native_compile_settings.py deleted file mode 100644 index 73e459818f7..00000000000 --- a/src/python/pants/backend/native/subsystems/native_compile_settings.py +++ /dev/null @@ -1,37 +0,0 @@ -# coding=utf-8 -# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md). -# Licensed under the Apache License, Version 2.0 (see LICENSE). - -from __future__ import absolute_import, division, print_function, unicode_literals - -from pants.subsystem.subsystem import Subsystem - - -class NativeCompileSettings(Subsystem): - """Any settings relevant to a compiler invocation.""" - - @classmethod - def register_options(cls, register): - super(NativeCompileSettings, cls).register_options(register) - - register('--strict-deps', type=bool, default=True, fingerprint=True, advanced=True, - help='The default for the "strict_deps" argument for targets of this language.') - register('--fatal-warnings', type=bool, default=True, fingerprint=True, advanced=True, - help='The default for the "fatal_warnings" argument for targets of this language.') - - # TODO: use some more formal method of mirroring options between a target and a subsystem -- see - # pants.backend.jvm.subsystems.dependency_context.DependencyContext#defaulted_property()! - def get_subsystem_target_mirrored_field_value(self, field_name, target): - """Get the attribute `field_name` from `target` if set, else from this subsystem's options.""" - tgt_setting = getattr(target, field_name) - if tgt_setting is None: - return getattr(self.get_options(), field_name) - return tgt_setting - - -class CCompileSettings(NativeCompileSettings): - options_scope = 'c-compile' - - -class CppCompileSettings(NativeCompileSettings): - options_scope = 'cpp-compile' diff --git a/src/python/pants/backend/native/subsystems/utils/mirrored_target_option_mixin.py b/src/python/pants/backend/native/subsystems/utils/mirrored_target_option_mixin.py new file mode 100644 index 00000000000..c91cba5f9c8 --- /dev/null +++ b/src/python/pants/backend/native/subsystems/utils/mirrored_target_option_mixin.py @@ -0,0 +1,35 @@ +# coding=utf-8 +# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md). +# Licensed under the Apache License, Version 2.0 (see LICENSE). + +from __future__ import absolute_import, division, print_function, unicode_literals + +from builtins import object + +from pants.util.meta import classproperty + + +# TODO: consider coalescing existing methods of mirroring options between a target and a subsystem +# -- see pants.backend.jvm.subsystems.dependency_context.DependencyContext#defaulted_property()! +class MirroredTargetOptionMixin(object): + """Get option values which may be set in this subsystem or in a Target's keyword argument.""" + + @classproperty + def mirrored_option_to_kwarg_map(cls): + """Subclasses should override and return a dict of (subsystem option name) -> (target kwarg). + + This classproperty should return a dict mapping this subsystem's options attribute name (with + underscores) to the corresponding target's keyword argument name. + """ + raise NotImplementedError() + + def get_target_mirrored_option(self, option_name, target): + field_name = self.mirrored_option_to_kwarg_map[option_name] + return self._get_subsystem_target_mirrored_field_value(option_name, field_name, target) + + def _get_subsystem_target_mirrored_field_value(self, option_name, field_name, target): + """Get the attribute `field_name` from `target` if set, else from this subsystem's options.""" + tgt_setting = getattr(target, field_name) + if tgt_setting is None: + return getattr(self.get_options(), option_name) + return tgt_setting diff --git a/src/python/pants/backend/native/tasks/c_compile.py b/src/python/pants/backend/native/tasks/c_compile.py index a1aea9a4919..a7b792024b4 100644 --- a/src/python/pants/backend/native/tasks/c_compile.py +++ b/src/python/pants/backend/native/tasks/c_compile.py @@ -5,11 +5,9 @@ from __future__ import absolute_import, division, print_function, unicode_literals from pants.backend.native.config.environment import LLVMCToolchain -from pants.backend.native.subsystems.native_compile_settings import CCompileSettings -from pants.backend.native.subsystems.native_toolchain import NativeToolchain +from pants.backend.native.subsystems.native_build_step_settings_base import CCompileSettings from pants.backend.native.targets.native_library import CLibrary from pants.backend.native.tasks.native_compile import NativeCompile -from pants.util.memo import memoized_property from pants.util.objects import SubclassesOf @@ -28,21 +26,10 @@ def implementation_version(cls): @classmethod def subsystem_dependencies(cls): - return super(CCompile, cls).subsystem_dependencies() + ( - CCompileSettings.scoped(cls), - NativeToolchain.scoped(cls), - ) - - @memoized_property - def _native_toolchain(self): - return NativeToolchain.scoped_instance(self) + return super(CCompile, cls).subsystem_dependencies() + (CCompileSettings.scoped(cls),) def get_compile_settings(self): return CCompileSettings.scoped_instance(self) - @memoized_property - def _c_toolchain(self): - return self._request_single(LLVMCToolchain, self._native_toolchain).c_toolchain - def get_compiler(self): - return self._c_toolchain.c_compiler + return self._request_single(LLVMCToolchain, self._native_toolchain).c_toolchain.c_compiler diff --git a/src/python/pants/backend/native/tasks/cpp_compile.py b/src/python/pants/backend/native/tasks/cpp_compile.py index 4d58348216d..632eb0bd047 100644 --- a/src/python/pants/backend/native/tasks/cpp_compile.py +++ b/src/python/pants/backend/native/tasks/cpp_compile.py @@ -5,11 +5,9 @@ from __future__ import absolute_import, division, print_function, unicode_literals from pants.backend.native.config.environment import LLVMCppToolchain -from pants.backend.native.subsystems.native_compile_settings import CppCompileSettings -from pants.backend.native.subsystems.native_toolchain import NativeToolchain +from pants.backend.native.subsystems.native_build_step_settings_base import CppCompileSettings from pants.backend.native.targets.native_library import CppLibrary from pants.backend.native.tasks.native_compile import NativeCompile -from pants.util.memo import memoized_property from pants.util.objects import SubclassesOf @@ -28,21 +26,10 @@ def implementation_version(cls): @classmethod def subsystem_dependencies(cls): - return super(CppCompile, cls).subsystem_dependencies() + ( - CppCompileSettings.scoped(cls), - NativeToolchain.scoped(cls), - ) - - @memoized_property - def _native_toolchain(self): - return NativeToolchain.scoped_instance(self) + return super(CppCompile, cls).subsystem_dependencies() + (CppCompileSettings.scoped(cls),) def get_compile_settings(self): return CppCompileSettings.scoped_instance(self) - @memoized_property - def _cpp_toolchain(self): - return self._request_single(LLVMCppToolchain, self._native_toolchain).cpp_toolchain - def get_compiler(self): - return self._cpp_toolchain.cpp_compiler + return self._request_single(LLVMCppToolchain, self._native_toolchain).cpp_toolchain.cpp_compiler 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 8e59f514039..66a2cf0dac7 100644 --- a/src/python/pants/backend/native/tasks/link_shared_libraries.py +++ b/src/python/pants/backend/native/tasks/link_shared_libraries.py @@ -7,17 +7,15 @@ import os from pants.backend.native.config.environment import Linker, LLVMCppToolchain, Platform -from pants.backend.native.subsystems.native_toolchain import NativeToolchain from pants.backend.native.targets.native_artifact import NativeArtifact from pants.backend.native.targets.native_library import NativeLibrary -from pants.backend.native.tasks.native_compile import NativeTargetDependencies, ObjectFiles +from pants.backend.native.tasks.native_compile import ObjectFiles from pants.backend.native.tasks.native_external_library_fetch import NativeExternalLibraryFiles from pants.backend.native.tasks.native_task import NativeTask from pants.base.exceptions import TaskError from pants.base.workunit import WorkUnit, WorkUnitLabel -from pants.util.collections import assert_single_element from pants.util.memo import memoized_property -from pants.util.objects import datatype +from pants.util.objects import SubclassesOf, datatype from pants.util.process_handler import subprocess @@ -39,6 +37,10 @@ class LinkSharedLibraries(NativeTask): options_scope = 'link-shared-libraries' + # TODO(#6486): change this to include ExternalNativeLibrary, then add a test that strict-deps + # works on external libs. + source_target_constraint = SubclassesOf(NativeLibrary) + @classmethod def product_types(cls): return [SharedLibrary] @@ -46,7 +48,6 @@ def product_types(cls): @classmethod def prepare(cls, options, round_manager): super(LinkSharedLibraries, cls).prepare(options, round_manager) - round_manager.require(NativeTargetDependencies) round_manager.require(ObjectFiles) round_manager.optional_product(NativeExternalLibraryFiles) @@ -56,18 +57,10 @@ def cache_target_dirs(self): @classmethod def implementation_version(cls): - return super(LinkSharedLibraries, cls).implementation_version() + [('LinkSharedLibraries', 0)] + return super(LinkSharedLibraries, cls).implementation_version() + [('LinkSharedLibraries', 1)] class LinkSharedLibrariesError(TaskError): pass - @classmethod - def subsystem_dependencies(cls): - return super(LinkSharedLibraries, cls).subsystem_dependencies() + (NativeToolchain.scoped(cls),) - - @memoized_property - def _native_toolchain(self): - return NativeToolchain.scoped_instance(self) - @memoized_property def _cpp_toolchain(self): return self._request_single(LLVMCppToolchain, self._native_toolchain).cpp_toolchain @@ -81,17 +74,8 @@ def platform(self): # TODO: convert this to a v2 engine dependency injection. return Platform.create() - def _retrieve_single_product_at_target_base(self, product_mapping, target): - self.context.log.debug("product_mapping: {}".format(product_mapping)) - self.context.log.debug("target: {}".format(target)) - product = product_mapping.get(target) - single_base_dir = assert_single_element(product.keys()) - single_product = assert_single_element(product[single_base_dir]) - return single_product - def execute(self): targets_providing_artifacts = self.context.targets(NativeLibrary.produces_ctypes_native_library) - native_target_deps_product = self.context.products.get(NativeTargetDependencies) compiled_objects_product = self.context.products.get(ObjectFiles) shared_libs_product = self.context.products.get(SharedLibrary) external_libs_product = self.context.products.get_data(NativeExternalLibraryFiles) @@ -108,7 +92,7 @@ def execute(self): # perform a link to every native_external_library for all targets in the closure. # https://github.com/pantsbuild/pants/issues/6178 link_request = self._make_link_request( - vt, compiled_objects_product, native_target_deps_product, external_libs_product) + vt, compiled_objects_product, external_libs_product) self.context.log.debug("link_request: {}".format(link_request)) shared_library = self._execute_link_request(link_request) @@ -123,7 +107,7 @@ def execute(self): else: all_shared_libs_by_name[shared_library.name] = shared_library - shared_libs_product.add(vt.target, vt.target.target_base).append(shared_library) + self._add_product_at_target_base(shared_libs_product, vt.target, shared_library) def _retrieve_shared_lib_from_cache(self, vt): native_artifact = vt.target.ctypes_native_library @@ -134,14 +118,10 @@ def _retrieve_shared_lib_from_cache(self, vt): .format(path_to_cached_lib)) return SharedLibrary(name=native_artifact.lib_name, path=path_to_cached_lib) - def _make_link_request(self, - vt, - compiled_objects_product, - native_target_deps_product, - external_libs_product): + def _make_link_request(self, vt, compiled_objects_product, external_libs_product): self.context.log.debug("link target: {}".format(vt.target)) - deps = self._retrieve_single_product_at_target_base(native_target_deps_product, vt.target) + deps = self.native_deps(vt.target) all_compiled_object_files = [] for dep_tgt in deps: @@ -174,13 +154,6 @@ def _make_link_request(self, 'linux': lambda: ['-shared'], } - def _get_third_party_lib_args(self, link_request): - ext_libs = link_request.external_libs_info - if not ext_libs: - return [] - - return ext_libs.get_third_party_lib_args() - def _execute_link_request(self, link_request): object_files = link_request.object_files @@ -199,6 +172,8 @@ def _execute_link_request(self, link_request): self.platform.resolve_platform_specific(self._SHARED_CMDLINE_ARGS) + linker.extra_args + ['-o', os.path.abspath(resulting_shared_lib_path)] + + ['-L{}'.format(lib_dir) for lib_dir in link_request.external_lib_dirs] + + ['-l{}'.format(lib_name) for lib_name in link_request.external_lib_names] + [os.path.abspath(obj) for obj in object_files]) self.context.log.debug("linker command: {}".format(cmd)) diff --git a/src/python/pants/backend/native/tasks/native_compile.py b/src/python/pants/backend/native/tasks/native_compile.py index 8706fdbaa06..2e957170a38 100644 --- a/src/python/pants/backend/native/tasks/native_compile.py +++ b/src/python/pants/backend/native/tasks/native_compile.py @@ -6,7 +6,6 @@ import os from abc import abstractmethod -from builtins import filter from collections import defaultdict from pants.backend.native.config.environment import Executable @@ -17,9 +16,8 @@ from pants.base.build_environment import get_buildroot from pants.base.exceptions import TaskError from pants.base.workunit import WorkUnit, WorkUnitLabel -from pants.build_graph.dependency_context import DependencyContext from pants.util.memo import memoized_method, memoized_property -from pants.util.meta import AbstractClass +from pants.util.meta import AbstractClass, classproperty from pants.util.objects import SubclassesOf, datatype from pants.util.process_handler import subprocess @@ -41,11 +39,6 @@ def file_paths(self): return [os.path.join(self.root_dir, fname) for fname in self.filenames] -# TODO: this is a temporary hack -- we could introduce something like a "NativeRequirement" with -# dependencies, header, object file, library name (more?) instead of using multiple products. -class NativeTargetDependencies(datatype(['native_deps'])): pass - - class NativeCompile(NativeTask, AbstractClass): # `NativeCompile` will use the `source_target_constraint` to determine what targets have "sources" # to compile, and the `dependent_target_constraint` to determine which dependent targets to @@ -56,11 +49,13 @@ class NativeCompile(NativeTask, AbstractClass): # `NativeCompile` will use `workunit_label` as the name of the workunit when executing the # compiler process. `workunit_label` must be set to a string. - workunit_label = None + @classproperty + def workunit_label(cls): + raise NotImplementedError('subclasses of NativeCompile must override workunit_label!') @classmethod def product_types(cls): - return [ObjectFiles, NativeTargetDependencies] + return [ObjectFiles] @classmethod def prepare(cls, options, round_manager): @@ -71,20 +66,9 @@ def prepare(cls, options, round_manager): def cache_target_dirs(self): return True - @abstractmethod - def get_compile_settings(self): - """An instance of `NativeCompileSettings` which is used in `NativeCompile`. - - :return: :class:`pants.backend.native.subsystems.native_compile_settings.NativeCompileSettings` - """ - - @memoized_property - def _compile_settings(self): - return self.get_compile_settings() - @classmethod def implementation_version(cls): - return super(NativeCompile, cls).implementation_version() + [('NativeCompile', 0)] + return super(NativeCompile, cls).implementation_version() + [('NativeCompile', 1)] class NativeCompileError(TaskError): """Raised for errors in this class's logic. @@ -92,46 +76,14 @@ class NativeCompileError(TaskError): Subclasses are advised to create their own exception class. """ - def native_deps(self, target): - return self.strict_deps_for_target( - target, predicate=self.dependent_target_constraint.satisfied_by) - - def strict_deps_for_target(self, target, predicate=None): - """Get the dependencies of `target` filtered by `predicate`, accounting for 'strict_deps'. - - If 'strict_deps' is on, instead of using the transitive closure of dependencies, targets will - only be able to see their immediate dependencies declared in the BUILD file. The 'strict_deps' - setting is obtained from the result of `get_compile_settings()`. - - NB: This includes the current target in the result. - """ - if self._compile_settings.get_subsystem_target_mirrored_field_value('strict_deps', target): - strict_deps = target.strict_dependencies(DependencyContext()) - if predicate: - filtered_deps = list(filter(predicate, strict_deps)) - else: - filtered_deps = strict_deps - deps = [target] + filtered_deps - else: - deps = self.context.build_graph.transitive_subgraph_of_addresses( - [target.address], predicate=predicate) - - return deps - - @staticmethod - def _add_product_at_target_base(product_mapping, target, value): - product_mapping.add(target, target.target_base).append(value) - def execute(self): object_files_product = self.context.products.get(ObjectFiles) - native_deps_product = self.context.products.get(NativeTargetDependencies) external_libs_product = self.context.products.get_data(NativeExternalLibraryFiles) source_targets = self.context.targets(self.source_target_constraint.satisfied_by) with self.invalidated(source_targets, invalidate_dependents=True) as invalidation_check: for vt in invalidation_check.all_vts: deps = self.native_deps(vt.target) - self._add_product_at_target_base(native_deps_product, vt.target, deps) if not vt.valid: compile_request = self._make_compile_request(vt, deps, external_libs_product) self.context.log.debug("compile_request: {}".format(compile_request)) @@ -143,7 +95,7 @@ def execute(self): # This may be calculated many times for a target, so we memoize it. @memoized_method def _include_dirs_for_target(self, target): - return os.path.join(get_buildroot(), target.target_base) + return os.path.join(get_buildroot(), target.address.spec_path) class NativeSourcesByType(datatype(['rel_root', 'headers', 'sources'])): pass @@ -179,13 +131,23 @@ def get_sources_headers_for_target(self, target): return [os.path.join(get_buildroot(), rel_root, src) for src in target_relative_sources] - # TODO(#5951): expand `Executable` to cover argv generation (where an `Executable` is subclassed - # to modify or extend the argument list, as declaratively as possible) to remove - # `extra_compile_args(self)`! + @abstractmethod + def get_compile_settings(self): + """Return a subclass of NativeBuildStepSettingsBase. + + NB: Subclasses will be queried for the compile settings once and the result cached. + """ + + @memoized_property + def _compile_settings(self): + return self.get_compile_settings() + @abstractmethod def get_compiler(self): """An instance of `Executable` 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` """ @@ -213,8 +175,7 @@ def _make_compile_request(self, versioned_target, dependencies, external_libs_pr compiler=self._compiler, include_dirs=include_dirs, sources=sources_and_headers, - fatal_warnings=self._compile_settings.get_subsystem_target_mirrored_field_value( - 'fatal_warnings', target), + fatal_warnings=self._compile_settings.get_fatal_warnings_value_for_target(target), output_dir=versioned_target.results_dir) def _make_compile_argv(self, compile_request): diff --git a/src/python/pants/backend/native/tasks/native_task.py b/src/python/pants/backend/native/tasks/native_task.py index 9bb8a010b0d..fcf3a13eee0 100644 --- a/src/python/pants/backend/native/tasks/native_task.py +++ b/src/python/pants/backend/native/tasks/native_task.py @@ -4,11 +4,99 @@ from __future__ import absolute_import, division, print_function, unicode_literals +from builtins import filter + +from pants.backend.native.subsystems.native_build_settings import NativeBuildSettings +from pants.backend.native.subsystems.native_toolchain import NativeToolchain +from pants.backend.native.targets.native_library import NativeLibrary +from pants.build_graph.dependency_context import DependencyContext from pants.task.task import Task +from pants.util.collections import assert_single_element +from pants.util.memo import memoized_classproperty, memoized_property +from pants.util.meta import classproperty +from pants.util.objects import SubclassesOf class NativeTask(Task): + @classproperty + def source_target_constraint(cls): + """Return a type constraint which is evaluated to determine "source" targets for this task. + + This is used to make it clearer which tasks act on which targets, since the compile and link + tasks work on different target sets (just C and just C++ in the compile tasks, and both in the + link task). + + :return: :class:`pants.util.objects.TypeConstraint` + """ + raise NotImplementedError() + + @memoized_classproperty + def dependent_target_constraint(cls): + """Return a type contraint which is evaluated to determine dependencies for a target. + + This is used to make strict_deps() calculation automatic and declarative. + + :return: :class:`pants.util.objects.TypeConstraint` + """ + return SubclassesOf(NativeLibrary) + + @classmethod + def subsystem_dependencies(cls): + return super(NativeTask, cls).subsystem_dependencies() + ( + NativeBuildSettings.scoped(cls), + NativeToolchain.scoped(cls), + ) + + @classmethod + def implementation_version(cls): + return super(NativeTask, cls).implementation_version() + [('NativeTask', 0)] + + @memoized_property + def _native_build_settings(self): + return NativeBuildSettings.scoped_instance(self) + + @memoized_property + def _native_toolchain(self): + return NativeToolchain.scoped_instance(self) + + def native_deps(self, target): + return self.strict_deps_for_target( + target, predicate=self.dependent_target_constraint.satisfied_by) + + def strict_deps_for_target(self, target, predicate=None): + """Get the dependencies of `target` filtered by `predicate`, accounting for 'strict_deps'. + + If 'strict_deps' is on, instead of using the transitive closure of dependencies, targets will + only be able to see their immediate dependencies declared in the BUILD file. The 'strict_deps' + setting is obtained from the result of `get_compile_settings()`. + + NB: This includes the current target in the result. + """ + if self._native_build_settings.get_strict_deps_value_for_target(target): + strict_deps = target.strict_dependencies(DependencyContext()) + if predicate: + filtered_deps = list(filter(predicate, strict_deps)) + else: + filtered_deps = strict_deps + deps = [target] + filtered_deps + else: + deps = self.context.build_graph.transitive_subgraph_of_addresses( + [target.address], predicate=predicate) + + return deps + + def _add_product_at_target_base(self, product_mapping, target, value): + product_mapping.add(target, target.target_base).append(value) + + def _retrieve_single_product_at_target_base(self, product_mapping, target): + self.context.log.debug("product_mapping: {}".format(product_mapping)) + self.context.log.debug("target: {}".format(target)) + product = product_mapping.get(target) + single_base_dir = assert_single_element(product.keys()) + single_product = assert_single_element(product[single_base_dir]) + return single_product + # TODO(#5869): delete this when we can request Subsystems from options in tasks! def _request_single(self, product, subject): # NB: This is not supposed to be exposed to Tasks yet -- see #4769 to track the status of diff --git a/testprojects/src/python/python_distribution/ctypes_interop/BUILD b/testprojects/src/python/python_distribution/ctypes_interop/BUILD new file mode 100644 index 00000000000..c1d66519a3d --- /dev/null +++ b/testprojects/src/python/python_distribution/ctypes_interop/BUILD @@ -0,0 +1,23 @@ +# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md). +# Licensed under the Apache License, Version 2.0 (see LICENSE). + +python_dist( + sources=[ + 'setup.py', + 'ctypes_python_pkg/__init__.py', + 'ctypes_python_pkg/ctypes_wrapper.py', + ], + dependencies=[ + 'testprojects/src/python/python_distribution/ctypes_interop/some-more-math', + 'testprojects/src/python/python_distribution/ctypes_interop/wrapped-math', + ], +) + +python_binary( + name='bin', + source='main.py', + dependencies=[ + ':ctypes_interop', + ], + platforms=['current'], +) diff --git a/testprojects/src/python/python_distribution/ctypes_interop/__init__.py b/testprojects/src/python/python_distribution/ctypes_interop/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/testprojects/src/python/python_distribution/ctypes_interop/ctypes_python_pkg/__init__.py b/testprojects/src/python/python_distribution/ctypes_interop/ctypes_python_pkg/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/testprojects/src/python/python_distribution/ctypes_interop/ctypes_python_pkg/ctypes_wrapper.py b/testprojects/src/python/python_distribution/ctypes_interop/ctypes_python_pkg/ctypes_wrapper.py new file mode 100644 index 00000000000..81cde0246fb --- /dev/null +++ b/testprojects/src/python/python_distribution/ctypes_interop/ctypes_python_pkg/ctypes_wrapper.py @@ -0,0 +1,39 @@ +# coding=utf-8 +# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md). +# Licensed under the Apache License, Version 2.0 (see LICENSE). + +from __future__ import (absolute_import, division, generators, nested_scopes, print_function, + unicode_literals, with_statement) + +import ctypes +import logging +import os + + +logger = logging.getLogger(__name__) + + +def get_generated_shared_lib(lib_name): + # These are the same filenames as in setup.py. + filename = 'lib{}.so'.format(lib_name) + # The data files are in the root directory, but we are in ctypes_python_pkg/. + rel_path = os.path.join(os.path.dirname(__file__), '..', filename) + return os.path.normpath(rel_path) + + +cpp_math_lib_path = get_generated_shared_lib('some-more-math') +c_wrapped_math_lib_path = get_generated_shared_lib('wrapped-math') + +cpp_math_lib = ctypes.CDLL(cpp_math_lib_path) +c_wrapped_math_lib = ctypes.CDLL(c_wrapped_math_lib_path) + + +def f(x): + some_cpp_math_result = cpp_math_lib.add_two(x) + cpp_math_lib.multiply_by_three(x) + logger.debug('some_cpp_math_result: {}'.format(some_cpp_math_result)) + some_c_wrapped_math_result = ( + c_wrapped_math_lib.add_two(x) + + c_wrapped_math_lib.multiply_by_three(x) + + c_wrapped_math_lib.wrapped_function(x)) + logger.debug('some_c_wrapped_math_result: {}'.format(some_c_wrapped_math_result)) + return some_cpp_math_result * some_c_wrapped_math_result diff --git a/testprojects/src/python/python_distribution/ctypes_interop/main.py b/testprojects/src/python/python_distribution/ctypes_interop/main.py new file mode 100644 index 00000000000..b0f77a64ce9 --- /dev/null +++ b/testprojects/src/python/python_distribution/ctypes_interop/main.py @@ -0,0 +1,13 @@ +# coding=utf-8 +# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md). +# Licensed under the Apache License, Version 2.0 (see LICENSE). + +from __future__ import (absolute_import, division, generators, nested_scopes, print_function, + unicode_literals, with_statement) + +from ctypes_python_pkg.ctypes_wrapper import f + + +if __name__ == '__main__': + x = 3 + print('x={}, f(x)={}'.format(x, f(x))) diff --git a/testprojects/src/python/python_distribution/ctypes_interop/setup.py b/testprojects/src/python/python_distribution/ctypes_interop/setup.py new file mode 100644 index 00000000000..a20f02c2605 --- /dev/null +++ b/testprojects/src/python/python_distribution/ctypes_interop/setup.py @@ -0,0 +1,17 @@ +# coding=utf-8 +# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md). +# Licensed under the Apache License, Version 2.0 (see LICENSE). + +from __future__ import (absolute_import, division, generators, nested_scopes, print_function, + unicode_literals, with_statement) + +from setuptools import setup, find_packages + + +setup( + name='ctypes_interop_test', + version='0.0.1', + packages=find_packages(), + # Declare two files at the top-level directory (denoted by ''). + data_files=[('', ['libsome-more-math.so', 'libwrapped-math.so'])], +) diff --git a/testprojects/src/python/python_distribution/ctypes_interop/some-math/BUILD b/testprojects/src/python/python_distribution/ctypes_interop/some-math/BUILD new file mode 100644 index 00000000000..bdaaae68059 --- /dev/null +++ b/testprojects/src/python/python_distribution/ctypes_interop/some-math/BUILD @@ -0,0 +1 @@ +ctypes_compatible_c_library() diff --git a/testprojects/src/python/python_distribution/ctypes_interop/some-math/some_math.c b/testprojects/src/python/python_distribution/ctypes_interop/some-math/some_math.c new file mode 100644 index 00000000000..a5a28435c49 --- /dev/null +++ b/testprojects/src/python/python_distribution/ctypes_interop/some-math/some_math.c @@ -0,0 +1,3 @@ +#include "some_math.h" + +int add_two(int x) { return x + 2; } diff --git a/testprojects/src/python/python_distribution/ctypes_interop/some-math/some_math.h b/testprojects/src/python/python_distribution/ctypes_interop/some-math/some_math.h new file mode 100644 index 00000000000..af394ad0504 --- /dev/null +++ b/testprojects/src/python/python_distribution/ctypes_interop/some-math/some_math.h @@ -0,0 +1,6 @@ +#ifndef __SOME_MATH_H__ +#define __SOME_MATH_H__ + +int add_two(int); + +#endif diff --git a/testprojects/src/python/python_distribution/ctypes_interop/some-more-math/BUILD b/testprojects/src/python/python_distribution/ctypes_interop/some-more-math/BUILD new file mode 100644 index 00000000000..97d0f7c2687 --- /dev/null +++ b/testprojects/src/python/python_distribution/ctypes_interop/some-more-math/BUILD @@ -0,0 +1,6 @@ +ctypes_compatible_cpp_library( + dependencies=[ + 'testprojects/src/python/python_distribution/ctypes_interop/some-math', + ], + ctypes_native_library=native_artifact(lib_name='some-more-math'), +) diff --git a/testprojects/src/python/python_distribution/ctypes_interop/some-more-math/some_more_math.cpp b/testprojects/src/python/python_distribution/ctypes_interop/some-more-math/some_more_math.cpp new file mode 100644 index 00000000000..ef15aff91df --- /dev/null +++ b/testprojects/src/python/python_distribution/ctypes_interop/some-more-math/some_more_math.cpp @@ -0,0 +1,12 @@ +#ifdef __cplusplus +extern "C" { +#endif +#include "some_math.h" +#ifdef __cplusplus +} +#endif +#include "some_more_math.hpp" + +int mangled_function(int x) { return add_two(x) ^ 3; } + +extern "C" int multiply_by_three(int x) { return mangled_function(x * 3); } diff --git a/testprojects/src/python/python_distribution/ctypes_interop/some-more-math/some_more_math.hpp b/testprojects/src/python/python_distribution/ctypes_interop/some-more-math/some_more_math.hpp new file mode 100644 index 00000000000..bca3b0dd47a --- /dev/null +++ b/testprojects/src/python/python_distribution/ctypes_interop/some-more-math/some_more_math.hpp @@ -0,0 +1,14 @@ +#ifndef __SOME_MORE_MATH_HPP__ +#define __SOME_MORE_MATH_HPP__ + +int mangled_function(int); + +#ifdef __cplusplus +extern "C" { +#endif + int multiply_by_three(int); +#ifdef __cplusplus +} +#endif + +#endif diff --git a/testprojects/src/python/python_distribution/ctypes_interop/wrapped-math/BUILD b/testprojects/src/python/python_distribution/ctypes_interop/wrapped-math/BUILD new file mode 100644 index 00000000000..a1174229b04 --- /dev/null +++ b/testprojects/src/python/python_distribution/ctypes_interop/wrapped-math/BUILD @@ -0,0 +1,10 @@ +ctypes_compatible_c_library( + dependencies=[ + 'testprojects/src/python/python_distribution/ctypes_interop/some-more-math', + ], + ctypes_native_library=native_artifact(lib_name='wrapped-math'), + # Turning on strict_deps brings the transitive closure of all the native dependencies into the + # compile and link tasks. In this case, this target also implicitly depends on the "some-math" C + # library, depended on by the "some-more-math" C++ library which we explicitly depend on above. + strict_deps=False, +) diff --git a/testprojects/src/python/python_distribution/ctypes_interop/wrapped-math/wrapped_math.c b/testprojects/src/python/python_distribution/ctypes_interop/wrapped-math/wrapped_math.c new file mode 100644 index 00000000000..16076563e41 --- /dev/null +++ b/testprojects/src/python/python_distribution/ctypes_interop/wrapped-math/wrapped_math.c @@ -0,0 +1,5 @@ +#include "some_math.h" +#include "some_more_math.hpp" +#include "wrapped_math.h" + +int wrapped_function(int x) { return add_two(multiply_by_three(x)); }; diff --git a/testprojects/src/python/python_distribution/ctypes_interop/wrapped-math/wrapped_math.h b/testprojects/src/python/python_distribution/ctypes_interop/wrapped-math/wrapped_math.h new file mode 100644 index 00000000000..f6854b82400 --- /dev/null +++ b/testprojects/src/python/python_distribution/ctypes_interop/wrapped-math/wrapped_math.h @@ -0,0 +1,6 @@ +#ifndef __WRAPPED_MATH_H__ +#define __WRAPPED_MATH_H__ + +int wrapped_function(int); + +#endif diff --git a/tests/python/pants_test/backend/python/tasks/test_ctypes_integration.py b/tests/python/pants_test/backend/python/tasks/test_ctypes_integration.py index d15c8416ebf..47f5bcfc1dc 100644 --- a/tests/python/pants_test/backend/python/tasks/test_ctypes_integration.py +++ b/tests/python/pants_test/backend/python/tasks/test_ctypes_integration.py @@ -13,7 +13,7 @@ from pants.option.scope import GLOBAL_SCOPE_CONFIG_SECTION from pants.util.collections import assert_single_element from pants.util.contextutil import temporary_dir -from pants.util.dirutil import is_executable +from pants.util.dirutil import is_executable, read_file, safe_file_dump from pants.util.process_handler import subprocess from pants_test.backend.python.tasks.python_task_test_base import name_and_platform from pants_test.pants_run_integration_test import PantsRunIntegrationTest @@ -26,16 +26,18 @@ def invoke_pex_for_output(pex_file_to_run): class CTypesIntegrationTest(PantsRunIntegrationTest): _binary_target = 'testprojects/src/python/python_distribution/ctypes:bin' + _binary_interop_target_dir = 'testprojects/src/python/python_distribution/ctypes_interop' + _binary_target_with_interop = '{}:bin'.format(_binary_interop_target_dir) + _wrapped_math_build_file = os.path.join(_binary_interop_target_dir, 'wrapped-math', 'BUILD') _binary_target_with_third_party = ( 'testprojects/src/python/python_distribution/ctypes_with_third_party:bin_with_third_party' ) def test_ctypes_run(self): - pants_run = self.run_pants(command=['run', self._binary_target]) + pants_run = self.run_pants(command=['-q', 'run', self._binary_target]) self.assert_success(pants_run) - # This is the entire output from main.py. - self.assertIn('x=3, f(x)=17', pants_run.stdout_data) + self.assertEqual('x=3, f(x)=17\n', pants_run.stdout_data) def test_ctypes_binary(self): with temporary_dir() as tmp_dir: @@ -77,20 +79,52 @@ def test_ctypes_binary(self): binary_run_output = invoke_pex_for_output(pex) self.assertEqual('x=3, f(x)=17\n', binary_run_output) + def test_ctypes_native_language_interop(self): + # TODO: consider making this mock_buildroot/run_pants_with_workdir into a + # PantsRunIntegrationTest method! + with self.mock_buildroot( + dirs_to_copy=[self._binary_interop_target_dir]) as buildroot, buildroot.pushd(): + + # Replace strict_deps=False with nothing so we can override it (because target values for this + # option take precedence over subsystem options). + orig_wrapped_math_build = read_file(self._wrapped_math_build_file) + without_strict_deps_wrapped_math_build = re.sub( + 'strict_deps=False,', '', orig_wrapped_math_build) + safe_file_dump(self._wrapped_math_build_file, without_strict_deps_wrapped_math_build) + + # This should fail because it does not turn on strict_deps for a target which requires it. + pants_binary_strict_deps_failure = self.run_pants_with_workdir( + command=['binary', self._binary_target_with_interop], + # Explicitly set to True (although this is the default). + config={'native-build-settings': {'strict_deps': True}}, + workdir=os.path.join(buildroot.new_buildroot, '.pants.d'), + build_root=buildroot.new_buildroot) + self.assert_failure(pants_binary_strict_deps_failure) + self.assertIn("fatal error: 'some_math.h' file not found", + pants_binary_strict_deps_failure.stdout_data) + + pants_run_interop = self.run_pants(['-q', 'run', self._binary_target_with_interop], config={ + 'native-build-settings': { + 'strict_deps': False, + }, + }) + self.assert_success(pants_run_interop) + self.assertEqual('x=3, f(x)=299\n', pants_run_interop.stdout_data) + def test_ctypes_third_party_integration(self): pants_binary = self.run_pants(['binary', self._binary_target_with_third_party]) self.assert_success(pants_binary) - pants_run = self.run_pants(['run', self._binary_target_with_third_party]) + pants_run = self.run_pants(['-q', 'run', self._binary_target_with_third_party]) self.assert_success(pants_run) - self.assertIn('Test worked!', pants_run.stdout_data) + self.assertIn('Test worked!\n', pants_run.stdout_data) # Test cached run. pants_run = self.run_pants( - command=['run', self._binary_target_with_third_party] + command=['-q', 'run', self._binary_target_with_third_party] ) self.assert_success(pants_run) - self.assertIn('Test worked!', pants_run.stdout_data) + self.assertIn('Test worked!\n', pants_run.stdout_data) def test_pants_native_source_detection_for_local_ctypes_dists_for_current_platform_only(self): """Test that `./pants run` respects platforms when the closure contains native sources.