From 64e6511c8589c514405999a14a4143e5f29583ba Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Tue, 5 Feb 2019 14:21:49 -0800 Subject: [PATCH] Only lint the direct sources of a linted target. --- .../contrib/scrooge/tasks/scrooge_gen.py | 4 +-- .../contrib/scrooge/tasks/thrift_linter.py | 8 ++--- .../contrib/scrooge/tasks/thrift_util.py | 17 ++++----- .../scrooge/tasks/test_thrift_linter.py | 36 +++++++++++++++---- 4 files changed, 42 insertions(+), 23 deletions(-) diff --git a/contrib/scrooge/src/python/pants/contrib/scrooge/tasks/scrooge_gen.py b/contrib/scrooge/src/python/pants/contrib/scrooge/tasks/scrooge_gen.py index 7f7841c4455..d81b9512c7a 100644 --- a/contrib/scrooge/src/python/pants/contrib/scrooge/tasks/scrooge_gen.py +++ b/contrib/scrooge/src/python/pants/contrib/scrooge/tasks/scrooge_gen.py @@ -24,7 +24,7 @@ from pants.contrib.scrooge.tasks.java_thrift_library_fingerprint_strategy import \ JavaThriftLibraryFingerprintStrategy -from pants.contrib.scrooge.tasks.thrift_util import calculate_compile_sources +from pants.contrib.scrooge.tasks.thrift_util import calculate_include_paths class ScroogeGen(SimpleCodegenTask, NailgunTask): @@ -148,7 +148,7 @@ def execute_codegen(self, target, target_workdir): self.gen(partial_cmd, target, target_workdir) def gen(self, partial_cmd, target, target_workdir): - import_paths, _ = calculate_compile_sources([target], self.is_gentarget) + import_paths = calculate_include_paths([target], self.is_gentarget) args = list(partial_cmd.compiler_args) diff --git a/contrib/scrooge/src/python/pants/contrib/scrooge/tasks/thrift_linter.py b/contrib/scrooge/src/python/pants/contrib/scrooge/tasks/thrift_linter.py index 33733b2f89a..ab32008fd11 100644 --- a/contrib/scrooge/src/python/pants/contrib/scrooge/tasks/thrift_linter.py +++ b/contrib/scrooge/src/python/pants/contrib/scrooge/tasks/thrift_linter.py @@ -15,7 +15,7 @@ from pants.option.ranked_value import RankedValue from pants.task.lint_task_mixin import LintTaskMixin -from pants.contrib.scrooge.tasks.thrift_util import calculate_compile_sources +from pants.contrib.scrooge.tasks.thrift_util import calculate_include_paths class ThriftLintError(Exception): @@ -87,14 +87,14 @@ def _lint(self, target, classpath): if not self._is_strict(target): config_args.append('--ignore-errors') - include_paths , paths = calculate_compile_sources([target], self._is_thrift) + paths = list(target.sources_relative_to_buildroot()) + include_paths = calculate_include_paths([target], self._is_thrift) if target.include_paths: include_paths |= set(target.include_paths) for p in include_paths: config_args.extend(['--include-path', p]) - args = config_args + list(paths) - + args = config_args + paths # If runjava returns non-zero, this marks the workunit as a # FAILURE, and there is no way to wrap this here. diff --git a/contrib/scrooge/src/python/pants/contrib/scrooge/tasks/thrift_util.py b/contrib/scrooge/src/python/pants/contrib/scrooge/tasks/thrift_util.py index 436513a07ec..6dc89cf4d4e 100644 --- a/contrib/scrooge/src/python/pants/contrib/scrooge/tasks/thrift_util.py +++ b/contrib/scrooge/src/python/pants/contrib/scrooge/tasks/thrift_util.py @@ -57,22 +57,19 @@ def find_root_thrifts(basedirs, sources, log=None): return root_sources -def calculate_compile_sources(targets, is_thrift_target): - """Calculates the set of thrift source files that need to be compiled. - It does not exclude sources that are included in other sources. - - A tuple of (include basedirs, thrift sources) is returned. +def calculate_include_paths(targets, is_thrift_target): + """Calculates the set of import paths for the given targets. :targets: The targets to examine. :is_thrift_target: A predicate to pick out thrift targets for consideration in the analysis. + + :returns: Include basedirs for the target. """ basedirs = set() - sources = set() - def collect_sources(target): + def collect_paths(target): basedirs.add(target.target_base) - sources.update(target.sources_relative_to_buildroot()) for target in targets: - target.walk(collect_sources, predicate=is_thrift_target) - return basedirs, sources + target.walk(collect_paths, predicate=is_thrift_target) + return basedirs diff --git a/contrib/scrooge/tests/python/pants_test/contrib/scrooge/tasks/test_thrift_linter.py b/contrib/scrooge/tests/python/pants_test/contrib/scrooge/tasks/test_thrift_linter.py index c40c8bc0ebf..1bf4be95537 100644 --- a/contrib/scrooge/tests/python/pants_test/contrib/scrooge/tasks/test_thrift_linter.py +++ b/contrib/scrooge/tests/python/pants_test/contrib/scrooge/tasks/test_thrift_linter.py @@ -31,25 +31,47 @@ def alias_groups(cls): def task_type(cls): return ThriftLinter - @patch('pants.contrib.scrooge.tasks.thrift_linter.calculate_compile_sources') - def test_lint(self, mock_calculate_compile_sources): + @patch('pants.contrib.scrooge.tasks.thrift_linter.calculate_include_paths') + def test_lint(self, mock_calculate_include_paths): def get_default_jvm_options(): return self.task_type().get_jvm_options_default(self.context().options.for_global_scope()) - thrift_target = self.create_library('a', 'java_thrift_library', 'a', ['A.thrift']) + thrift_target = self.create_library('src/thrift/tweet', 'java_thrift_library', 'a', ['A.thrift']) task = self.create_task(self.context(target_roots=thrift_target)) self._prepare_mocks(task) expected_include_paths = ['src/thrift/users', 'src/thrift/tweet'] - expected_paths = ['src/thrift/tweet/a.thrift', 'src/thrift/tweet/b.thrift'] - mock_calculate_compile_sources.return_value = (expected_include_paths, expected_paths) + mock_calculate_include_paths.return_value = expected_include_paths task._lint(thrift_target, task.tool_classpath('scrooge-linter')) self._run_java_mock.assert_called_once_with( classpath='foo_classpath', main='com.twitter.scrooge.linter.Main', args=['--fatal-warnings', '--ignore-errors', '--include-path', 'src/thrift/users', - '--include-path', 'src/thrift/tweet', 'src/thrift/tweet/a.thrift', - 'src/thrift/tweet/b.thrift'], + '--include-path', 'src/thrift/tweet', 'src/thrift/tweet/A.thrift'], + jvm_options=get_default_jvm_options(), + workunit_labels=[WorkUnitLabel.COMPILER, WorkUnitLabel.SUPPRESS_LABEL]) + + @patch('pants.contrib.scrooge.tasks.thrift_linter.calculate_include_paths') + def test_lint_direct_only(self, mock_calculate_include_paths): + # Validate that we do lint only the direct sources of a target, rather than including the + # sources of its transitive deps. + + def get_default_jvm_options(): + return self.task_type().get_jvm_options_default(self.context().options.for_global_scope()) + + self.create_library('src/thrift/tweet', 'java_thrift_library', 'a', ['A.thrift']) + target_b = self.create_library('src/thrift/tweet', 'java_thrift_library', 'b', ['B.thrift'], dependencies=[':a']) + task = self.create_task(self.context(target_roots=target_b)) + self._prepare_mocks(task) + mock_calculate_include_paths.return_value = ['src/thrift/tweet'] + task._lint(target_b, task.tool_classpath('scrooge-linter')) + + # Confirm that we did not include the sources of the dependency. + self._run_java_mock.assert_called_once_with( + classpath='foo_classpath', + main='com.twitter.scrooge.linter.Main', + args=['--fatal-warnings', '--ignore-errors', + '--include-path', 'src/thrift/tweet', 'src/thrift/tweet/B.thrift'], jvm_options=get_default_jvm_options(), workunit_labels=[WorkUnitLabel.COMPILER, WorkUnitLabel.SUPPRESS_LABEL])