From 4beaf2be0cd57a7c64b1dbc66136ec214ef67fb8 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Wed, 21 Nov 2018 20:03:07 -0800 Subject: [PATCH 1/5] move header file extension selection to an option --- .../subsystems/native_build_step_settings.py | 36 ++++++++++++------- .../backend/native/tasks/native_compile.py | 33 +++++++++++------ 2 files changed, 46 insertions(+), 23 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..e352368a68a 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,42 @@ 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), ) + header_file_extensions_default = None + + @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..e6b1f7bcd19 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 @@ -181,9 +182,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) - - def _make_compile_argv(self, compile_request): + output_dir=versioned_target.results_dir, + header_file_extensions=self._compile_settings + .header_file_extensions) + + def _split_sources_and_headers(self, sources, header_file_extensions): + sources_for_compiler = [] + header_files = [] + for s in sources: + for ext in header_file_extensions: + if s.endswith(ext): + header_files.append(s) + break + else: + sources_for_compiler.append(s) + return (sources_for_compiler, header_files) + + def _make_compile_argv(self, compile_request, sources_minus_headers): """Return a list of arguments to use to compile sources. Subclasses can override and append.""" compiler = compile_request.compiler compiler_options = compile_request.compiler_options @@ -199,7 +214,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 +226,16 @@ 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 + sources, _ = self._split_sources_and_headers( + compile_request.sources, compile_request.header_file_extensions) - 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)) + if len(sources) == 0: return compiler = compile_request.compiler output_dir = compile_request.output_dir - argv = self._make_compile_argv(compile_request) + argv = self._make_compile_argv(compile_request, sources) env = compiler.as_invocation_environment_dict with self.context.new_workunit( From d242d224f621dde52e26344aa09715eeeb029295 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Wed, 21 Nov 2018 20:56:01 -0800 Subject: [PATCH 2/5] add testing for header file overrides --- .../backend/native/tasks/native_compile.py | 3 +- .../backend/native/tasks/test_c_compile.py | 31 +++++++++++++++++++ .../backend/native/tasks/test_cpp_compile.py | 31 +++++++++++++++++++ 3 files changed, 63 insertions(+), 2 deletions(-) diff --git a/src/python/pants/backend/native/tasks/native_compile.py b/src/python/pants/backend/native/tasks/native_compile.py index e6b1f7bcd19..e4311073fc1 100644 --- a/src/python/pants/backend/native/tasks/native_compile.py +++ b/src/python/pants/backend/native/tasks/native_compile.py @@ -48,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 @@ -230,6 +228,7 @@ def _compile(self, compile_request): compile_request.sources, compile_request.header_file_extensions) if len(sources) == 0: + self.context.log.info('{} is a header-only library'.format(compile_request)) return compiler = compile_request.compiler 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..fd3526ff651 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 @@ -4,6 +4,7 @@ from __future__ import absolute_import, division, print_function, unicode_literals +import logging from textwrap import dedent from pants.backend.native.targets.native_library import CLibrary @@ -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,6 +54,21 @@ 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], + }, + }) + c_compile = self.create_task(context) + + with self.captured_logging(level=logging.INFO) as logs: + c_compile.execute() + info = list(logs.infos())[-1] + self.assertIn('is a header-only library', info) + def test_caching(self): c = self.create_simple_c_library() context = self.prepare_context_for_compile(target_roots=[c]) 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..c106e4d5142 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,6 +4,10 @@ from __future__ import absolute_import, division, print_function, unicode_literals +import logging +from textwrap import dedent + +from pants.backend.native.targets.native_library import CppLibrary from pants.backend.native.tasks.cpp_compile import CppCompile from pants_test.backend.native.tasks.native_task_test_base import (NativeCompileTestMixin, NativeTaskTestBase) @@ -14,6 +18,33 @@ class CppCompileTest(NativeTaskTestBase, NativeCompileTestMixin): def task_type(cls): return CppCompile + def create_simple_header_only_library(self, **kwargs): + 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]) + cpp_compile = self.create_task(context) + + with self.captured_logging(level=logging.INFO) as logs: + cpp_compile.execute() + info = list(logs.infos())[-1] + self.assertIn('is a header-only library', info) + def test_caching(self): cpp = self.create_simple_cpp_library() context = self.prepare_context_for_compile(target_roots=[cpp]) From 069d37ba98d644d5a7d27420bccde201ae8022a2 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Mon, 26 Nov 2018 17:36:03 -0800 Subject: [PATCH 3/5] make noop tests avoid the use of logging --- .../native/tasks/native_task_test_base.py | 11 +++++++++++ .../backend/native/tasks/test_c_compile.py | 14 +++++++++----- .../backend/native/tasks/test_cpp_compile.py | 17 ++++++++++++----- 3 files changed, 32 insertions(+), 10 deletions(-) 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 fd3526ff651..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 @@ -4,11 +4,11 @@ from __future__ import absolute_import, division, print_function, unicode_literals -import logging from textwrap import dedent 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) @@ -62,17 +62,21 @@ def test_header_only_noop_with_alternate_header_extension(self): '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() - with self.captured_logging(level=logging.INFO) as logs: - c_compile.execute() - info = list(logs.infos())[-1] - self.assertIn('is a header-only library', info) + 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 c106e4d5142..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,11 +4,11 @@ from __future__ import absolute_import, division, print_function, unicode_literals -import logging 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) @@ -19,6 +19,9 @@ 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__ @@ -38,17 +41,21 @@ def create_simple_header_only_library(self, **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() - with self.captured_logging(level=logging.INFO) as logs: - cpp_compile.execute() - info = list(logs.infos())[-1] - self.assertIn('is a header-only library', info) + 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() From 1f4acde2213337da018de406fcd5f6ba0bfdefa5 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Mon, 26 Nov 2018 17:38:46 -0800 Subject: [PATCH 4/5] make header_file_extensions_default a real classy property --- .../backend/native/subsystems/native_build_step_settings.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 e352368a68a..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 @@ -45,7 +45,9 @@ def subsystem_dependencies(cls): NativeBuildStepSettings.scoped(cls), ) - header_file_extensions_default = None + @classproperty + def header_file_extensions_default(cls): + raise NotImplementedError('header_file_extensions_default() must be overridden!') @classmethod def register_options(cls, register): From df833e050c1e772d5c22782f48aebc91ad1e19c0 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Mon, 26 Nov 2018 17:39:22 -0800 Subject: [PATCH 5/5] various edits in response to review comments on this file --- .../backend/native/tasks/native_compile.py | 41 +++++++++---------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/src/python/pants/backend/native/tasks/native_compile.py b/src/python/pants/backend/native/tasks/native_compile.py index e4311073fc1..1f57804d940 100644 --- a/src/python/pants/backend/native/tasks/native_compile.py +++ b/src/python/pants/backend/native/tasks/native_compile.py @@ -181,23 +181,22 @@ def _make_compile_request(self, versioned_target, dependencies, external_libs_pr .native_build_step_settings .get_merged_args_for_compiler_option_sets(compiler_option_sets)), output_dir=versioned_target.results_dir, - header_file_extensions=self._compile_settings - .header_file_extensions) - - def _split_sources_and_headers(self, sources, header_file_extensions): - sources_for_compiler = [] - header_files = [] - for s in sources: - for ext in header_file_extensions: - if s.endswith(ext): - header_files.append(s) - break - else: - sources_for_compiler.append(s) - return (sources_for_compiler, header_files) - - def _make_compile_argv(self, compile_request, sources_minus_headers): + 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. @@ -224,17 +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, _ = self._split_sources_and_headers( - compile_request.sources, compile_request.header_file_extensions) - if len(sources) == 0: - self.context.log.info('{} is a header-only library'.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, sources) env = compiler.as_invocation_environment_dict with self.context.new_workunit(