Skip to content

Commit

Permalink
Only lint the direct sources of a linted target. (pantsbuild#7219)
Browse files Browse the repository at this point in the history
### Problem

The thrift linter currently redundantly lints the transitive dependencies of each target, leading to repetitive errors, and larger tool invokes than necessary.

### Solution

Lint only the directly owned sources of a target, and expand unit tests.
  • Loading branch information
Stu Hood authored Feb 6, 2019
1 parent 121f98c commit 95638d3
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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])

0 comments on commit 95638d3

Please sign in to comment.