From ca9405c5b19878c7ae0aed1c606983bce910f420 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Mon, 26 Nov 2018 21:31:33 -0800 Subject: [PATCH] Header file extensions as options for C/C++ targets (#6802) ### Problem The file extensions we consider "header files" for C/C++ targets are hardcoded as `('.h', '.hpp')`. Additionally, we will error out on header-only `ctypes_compatible_cpp_library()` targets (as for C targets). ### Solution - Create the `--header-file-extensions` option in the CCompileSettings and CppCompileSettings subsystems. - Make a debug-level log if the library undergoing compile is a header-only library before early returning. - Add some tests. --- .../subsystems/native_build_step_settings.py | 38 ++++++++++++------- .../backend/native/tasks/native_compile.py | 31 +++++++++------ .../native/tasks/native_task_test_base.py | 11 ++++++ .../backend/native/tasks/test_c_compile.py | 35 +++++++++++++++++ .../backend/native/tasks/test_cpp_compile.py | 38 +++++++++++++++++++ 5 files changed, 129 insertions(+), 24 deletions(-) diff --git a/src/python/pants/backend/native/subsystems/native_build_step_settings.py b/src/python/pants/backend/native/subsystems/native_build_step_settings.py index baad65ed984..cf3a10f16b2 100644 --- a/src/python/pants/backend/native/subsystems/native_build_step_settings.py +++ b/src/python/pants/backend/native/subsystems/native_build_step_settings.py @@ -37,32 +37,44 @@ def get_compiler_option_sets_enabled_default_value(cls): return {"fatal_warnings": ["-Werror"]} -class CCompileSettings(Subsystem): - options_scope = 'c-compile-settings' +class CompileSettingsBase(Subsystem): @classmethod def subsystem_dependencies(cls): - return super(CCompileSettings, cls).subsystem_dependencies() + ( + return super(CompileSettingsBase, cls).subsystem_dependencies() + ( NativeBuildStepSettings.scoped(cls), ) + @classproperty + def header_file_extensions_default(cls): + raise NotImplementedError('header_file_extensions_default() must be overridden!') + + @classmethod + def register_options(cls, register): + super(CompileSettingsBase, cls).register_options(register) + register('--header-file-extensions', advanced=True, default=cls.header_file_extensions_default, + type=list, fingerprint=True, + help="The file extensions which should not be provided to the compiler command line.") + @memoized_property def native_build_step_settings(self): return NativeBuildStepSettings.scoped_instance(self) + @memoized_property + def header_file_extensions(self): + return self.get_options().header_file_extensions -class CppCompileSettings(Subsystem): - options_scope = 'cpp-compile-settings' - @classmethod - def subsystem_dependencies(cls): - return super(CppCompileSettings, cls).subsystem_dependencies() + ( - NativeBuildStepSettings.scoped(cls), - ) +class CCompileSettings(CompileSettingsBase): + options_scope = 'c-compile-settings' - @memoized_property - def native_build_step_settings(self): - return NativeBuildStepSettings.scoped_instance(self) + header_file_extensions_default = ['.h'] + + +class CppCompileSettings(CompileSettingsBase): + options_scope = 'cpp-compile-settings' + + header_file_extensions_default = ['.h', '.hpp', '.hxx', '.tpp'] # TODO: add a fatal_warnings kwarg to NativeArtifact and make a LinkSharedLibrariesSettings subclass diff --git a/src/python/pants/backend/native/tasks/native_compile.py b/src/python/pants/backend/native/tasks/native_compile.py index 95057fe1f86..1f57804d940 100644 --- a/src/python/pants/backend/native/tasks/native_compile.py +++ b/src/python/pants/backend/native/tasks/native_compile.py @@ -29,6 +29,7 @@ class NativeCompileRequest(datatype([ 'sources', 'compiler_options', 'output_dir', + 'header_file_extensions', ])): pass @@ -47,8 +48,6 @@ class NativeCompile(NativeTask, AbstractClass): source_target_constraint = None dependent_target_constraint = SubclassesOf(ExternalNativeLibrary, NativeLibrary) - HEADER_EXTENSIONS = ('.h', '.hpp') - # `NativeCompile` will use `workunit_label` as the name of the workunit when executing the # compiler process. `workunit_label` must be set to a string. @classproperty @@ -181,10 +180,23 @@ def _make_compile_request(self, versioned_target, dependencies, external_libs_pr compiler_options=(self._compile_settings .native_build_step_settings .get_merged_args_for_compiler_option_sets(compiler_option_sets)), - output_dir=versioned_target.results_dir) + output_dir=versioned_target.results_dir, + header_file_extensions=self._compile_settings.header_file_extensions) + + def _iter_sources_minus_headers(self, compile_request): + for s in compile_request.sources: + if not s.endswith(tuple(compile_request.header_file_extensions)): + yield s + + class _HeaderOnlyLibrary(Exception): pass def _make_compile_argv(self, compile_request): """Return a list of arguments to use to compile sources. Subclasses can override and append.""" + + sources_minus_headers = list(self._iter_sources_minus_headers(compile_request)) + if len(sources_minus_headers) == 0: + raise self._HeaderOnlyLibrary() + compiler = compile_request.compiler compiler_options = compile_request.compiler_options # We are going to execute in the target output, so get absolute paths for everything. @@ -199,7 +211,7 @@ def _make_compile_argv(self, compile_request): '-I{}'.format(os.path.join(buildroot, inc_dir)) for inc_dir in compile_request.include_dirs ] + - [os.path.join(buildroot, src) for src in compile_request.sources]) + [os.path.join(buildroot, src) for src in sources_minus_headers]) self.context.log.debug("compile argv: {}".format(argv)) @@ -211,18 +223,15 @@ def _compile(self, compile_request): NB: This method must arrange the output files so that `collect_cached_objects()` can collect all of the results (or vice versa)! """ - sources = compile_request.sources - if len(sources) == 0 and not any(s for s in sources if not s.endswith(self.HEADER_EXTENSIONS)): - # TODO: do we need this log message? Should we still have it for intentionally header-only - # libraries (that might be a confusing message to see)? - self.context.log.debug("no sources in request {}, skipping".format(compile_request)) + try: + argv = self._make_compile_argv(compile_request) + except self._HeaderOnlyLibrary: + self.context.log.debug('{} is a header-only library'.format(compile_request)) return compiler = compile_request.compiler output_dir = compile_request.output_dir - - argv = self._make_compile_argv(compile_request) env = compiler.as_invocation_environment_dict with self.context.new_workunit( diff --git a/tests/python/pants_test/backend/native/tasks/native_task_test_base.py b/tests/python/pants_test/backend/native/tasks/native_task_test_base.py index 2224073da45..21cc0a0107e 100644 --- a/tests/python/pants_test/backend/native/tasks/native_task_test_base.py +++ b/tests/python/pants_test/backend/native/tasks/native_task_test_base.py @@ -20,6 +20,17 @@ def rules(cls): class NativeCompileTestMixin(object): + + def _retrieve_single_product_at_target_base(self, product_mapping, target): + product = product_mapping.get(target) + base_dirs = list(product.keys()) + self.assertEqual(1, len(base_dirs)) + single_base_dir = base_dirs[0] + all_products = product[single_base_dir] + self.assertEqual(1, len(all_products)) + single_product = all_products[0] + return single_product + def create_simple_cpp_library(self, **kwargs): self.create_file('src/cpp/test/test.hpp', contents=dedent(""" #ifndef __TEST_HPP__ diff --git a/tests/python/pants_test/backend/native/tasks/test_c_compile.py b/tests/python/pants_test/backend/native/tasks/test_c_compile.py index bcdd0bcf6ba..7abb72b9529 100644 --- a/tests/python/pants_test/backend/native/tasks/test_c_compile.py +++ b/tests/python/pants_test/backend/native/tasks/test_c_compile.py @@ -8,6 +8,7 @@ from pants.backend.native.targets.native_library import CLibrary from pants.backend.native.tasks.c_compile import CCompile +from pants.backend.native.tasks.native_compile import ObjectFiles from pants_test.backend.native.tasks.native_task_test_base import (NativeCompileTestMixin, NativeTaskTestBase) @@ -17,6 +18,21 @@ class CCompileTest(NativeTaskTestBase, NativeCompileTestMixin): def task_type(cls): return CCompile + def create_header_only_alternate_c_library(self, ext, **kwargs): + header_filename = 'test{}'.format(ext) + self.create_file('src/c/test/{}'.format(header_filename), contents=dedent(""" + #ifndef __TEST_H__ + #define __TEST_H__ + + int test(int); + + #endif +""")) + return self.make_target(spec='src/c/test', + target_type=CLibrary, + sources=[header_filename], + **kwargs) + def create_simple_c_library(self, **kwargs): self.create_file('src/c/test/test.h', contents=dedent(""" #ifndef __TEST_H__ @@ -38,10 +54,29 @@ def create_simple_c_library(self, **kwargs): sources=['test.h', 'test.c'], **kwargs) + def test_header_only_noop_with_alternate_header_extension(self): + alternate_extension = '.asdf' + c = self.create_header_only_alternate_c_library(alternate_extension) + context = self.prepare_context_for_compile(target_roots=[c], options={ + 'c-compile-settings': { + 'header_file_extensions': [alternate_extension], + }, + }) + + # Test that the task runs without error if provided a header-only library. + c_compile = self.create_task(context) + c_compile.execute() + + object_files_product = context.products.get(ObjectFiles) + object_files_for_target = self._retrieve_single_product_at_target_base(object_files_product, c) + # Test that no object files were produced. + self.assertEqual(0, len(object_files_for_target.filenames)) + def test_caching(self): c = self.create_simple_c_library() context = self.prepare_context_for_compile(target_roots=[c]) c_compile = self.create_task(context) + # TODO: what is this testing? c_compile.execute() c_compile.execute() diff --git a/tests/python/pants_test/backend/native/tasks/test_cpp_compile.py b/tests/python/pants_test/backend/native/tasks/test_cpp_compile.py index 77286d6e398..40ddee23c0d 100644 --- a/tests/python/pants_test/backend/native/tasks/test_cpp_compile.py +++ b/tests/python/pants_test/backend/native/tasks/test_cpp_compile.py @@ -4,7 +4,11 @@ from __future__ import absolute_import, division, print_function, unicode_literals +from textwrap import dedent + +from pants.backend.native.targets.native_library import CppLibrary from pants.backend.native.tasks.cpp_compile import CppCompile +from pants.backend.native.tasks.native_compile import ObjectFiles from pants_test.backend.native.tasks.native_task_test_base import (NativeCompileTestMixin, NativeTaskTestBase) @@ -14,10 +18,44 @@ class CppCompileTest(NativeTaskTestBase, NativeCompileTestMixin): def task_type(cls): return CppCompile + def create_simple_header_only_library(self, **kwargs): + # TODO: determine if there are other features that people expect from header-only libraries that + # we could be testing here. Currently this file's contents are just C++ which doesn't define any + # non-template classes or methods. + self.create_file('src/cpp/test/test.hpp', contents=dedent(""" + #ifndef __TEST_HPP__ + #define __TEST_HPP__ + + template + T add(T a, T b) { + return a + b; + } + + #endif +""")) + return self.make_target(spec='src/cpp/test', + target_type=CppLibrary, + sources=['test.hpp'], + **kwargs) + + def test_header_only_target_noop(self): + cpp = self.create_simple_header_only_library() + context = self.prepare_context_for_compile(target_roots=[cpp]) + + # Test that the task runs without error if provided a header-only library. + cpp_compile = self.create_task(context) + cpp_compile.execute() + + object_files_product = context.products.get(ObjectFiles) + object_files_for_target = self._retrieve_single_product_at_target_base(object_files_product, cpp) + # Test that no object files were produced. + self.assertEqual(0, len(object_files_for_target.filenames)) + def test_caching(self): cpp = self.create_simple_cpp_library() context = self.prepare_context_for_compile(target_roots=[cpp]) cpp_compile = self.create_task(context) + # TODO: what is this testing? cpp_compile.execute() cpp_compile.execute()