Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only thrift-lint the direct sources of a linted target. #7219

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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])