Skip to content

Commit

Permalink
Implement zinc unused deps check
Browse files Browse the repository at this point in the history
This is an implementation of an "unused deps" check for zinc. See https://docs.google.com/document/d/1WfdcP6GE_4eDUZKGfaMcMJonhXxbUQtaNUMxz_7Gi5c/edit# for more information.

- Add a `forced` scope to indicate that a particular target is in the `default` scope, but is not eligible for unused dep checks.
- Separate JvmDependencyAnalyzer out of the Task hierarchy into a standalone class.
- Implement the check and return a dict of suggested (ie, more specific) replacements.
- Move strict/non-strict dep calculation into CompileContext.
- Remove spurious deps in the `examples` code.
- Remove `strict_deps=False` in a few places where `scope=forced` on a particular dep would be preferable.
- Formalize JavacPlugin's `tools.jar` dep as an injected target provided by a task during `bootstrap`.
- In case of an unused dep (which is eligible to be marked that way due to its scope), declare the dependency_type of the edge `unused` in the `dep-usage.jvm` task.

This review also cleans up all warnings/errors for:

    ./pants compile.zinc --unused-deps=fatal examples/{src,tests}/{java,scala}/::

When an unused dep is encountered, it is logged like so:

    compile(examples/tests/scala/org/pantsbuild/example/hello/welcome:welcome) failed: unused dependencies:
      'examples/src/scala/org/pantsbuild/example/hello/exe:exe',
    Suggested replacements:
      'examples/src/scala/org/pantsbuild/example/hello/welcome:welcome',
    (If you're seeing this message in error, you might need to change the `scope` of the dependencies.)

... with suggested replacements coming from the transitive deps of the unused deps that _were_ used.

Testing Done:
http://jenkins.pantsbuild.org/job/pantsbuild/job/pants/job/PR-3115/11/

Bugs closed: 3115

Reviewed at https://rbcommons.com/s/twitter/r/3635/
  • Loading branch information
stuhood committed Jun 28, 2016
1 parent fa29329 commit 5b7d317
Show file tree
Hide file tree
Showing 42 changed files with 565 additions and 273 deletions.
28 changes: 18 additions & 10 deletions 3rdparty/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@ jar_library(name='log4j',
jar_library(name='protobuf-2.4.1',
jars = [
jar(org='com.google.protobuf', name='protobuf-java', rev='2.4.1'),
])
],
# javac requires protobuf to be present in order to compile the MessageBuilders API.
scope='forced',)

target(name='protobuf-java', dependencies = [':protobuf-2.4.1'])

Expand All @@ -98,18 +100,15 @@ jar_library(name='slf4j-api',
])

# NB: we have two versions of thrift here due to scrooge requiring one version while apache
# thrift requires another. this is not usually recommended
# thrift requires another. This is not usually recommended.
jar_library(name='libthrift-0.9.2',
jars = [
jar(org='org.apache.thrift', name='libthrift', rev='0.9.2')
])
],
# javac uses undeclared types to build the MessageBuilder API.
scope='forced',)

target(name='thrift-0.9.2',
dependencies = [
':commons-lang',
':libthrift-0.9.2',
':slf4j-api',
])
target(name='thrift-0.9.2', dependencies = [':libthrift-0.9.2'])

jar_library(name='libthrift-0.6.1',
jars = [
Expand Down Expand Up @@ -146,9 +145,18 @@ jar_library(name='guava-testlib',
jar_library(name='junit',
jars=[
jar('junit', 'junit', '4.12')
])
],
# junit is frequently used only for its annotations.
scope='forced',)

jar_library(name='scalatest',
jars=[
scala_jar('org.scalatest', 'scalatest', '2.2.4')
])

jar_library(name='wire-runtime',
jars=[
jar(org='com.squareup.wire', name='wire-runtime', rev='1.8.0'),
],
# javac uses undeclared types to build the MessageBuilder API.
scope='forced',)
2 changes: 2 additions & 0 deletions 3rdparty/jvm/com/google/auto/value/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,6 @@ jar_library(name='value',
jars=[
jar(org='com.google.auto.value', name='auto-value', rev='1.1'),
],
# Is used as an annotation processor.
scope='compile',
)
5 changes: 5 additions & 0 deletions contrib/go/src/python/pants/contrib/go/tasks/go_thrift_gen.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,11 @@ def product_types(cls):
def execute_codegen(self, target, target_workdir):
self._generate_thrift(target, target_workdir)

@property
def _copy_target_attributes(self):
"""Override `_copy_target_attributes` to exclude `provides`."""
return [a for a in super(GoThriftGen, self)._copy_target_attributes if a != 'provides']

def synthetic_target_dir(self, target, target_workdir):
all_sources = list(target.sources_relative_to_buildroot())
source = all_sources[0]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from pants.backend.codegen.tasks.simple_codegen_task import SimpleCodegenTask
from pants.backend.jvm.tasks.nailgun_task import NailgunTask
from pants.base.exceptions import TargetDefinitionException, TaskError
from pants.build_graph.address import Address
from pants.build_graph.address_lookup_error import AddressLookupError
from pants.util.dirutil import safe_mkdir, safe_open
from pants.util.memo import memoized_method, memoized_property
Expand Down Expand Up @@ -60,7 +61,7 @@ def product_types(cls):

@classmethod
def implementation_version(cls):
return super(ScroogeGen, cls).implementation_version() + [('ScroogeGen', 2)]
return super(ScroogeGen, cls).implementation_version() + [('ScroogeGen', 3)]

def __init__(self, *args, **kwargs):
super(ScroogeGen, self).__init__(*args, **kwargs)
Expand All @@ -83,8 +84,10 @@ def _resolve_deps(self, depmap):
for category, depspecs in depmap.items():
dependencies = deps[category]
for depspec in depspecs:
dep_address = Address.parse(depspec)
try:
dependencies.update(self.context.resolve(depspec))
self.context.build_graph.maybe_inject_address_closure(dep_address)
dependencies.add(self.context.build_graph.get_target(dep_address))
except AddressLookupError as e:
raise AddressLookupError('{}\n referenced from {} scope'.format(e, self.options_scope))
return deps
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@

java_library(name='example',
sources=['Example.java'],
dependencies=[
],
# Is used by an annotation processor.
scope='compile',
)
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ annotation_processor(name='processor',
'examples/src/java/org/pantsbuild/example/annotation/example',
'3rdparty:guava',
],
scope='forced',
)

1 change: 1 addition & 0 deletions examples/src/java/org/pantsbuild/example/plugin/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ javac_plugin(
sources = ['SimpleJavacPlugin.java'],
dependencies = [],
classname = 'org.pantsbuild.example.plugin.SimpleJavacPlugin',
scope='compile',
)

java_library(
Expand Down
6 changes: 2 additions & 4 deletions examples/src/java/org/pantsbuild/example/wire/element/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,10 @@
jvm_binary(name='element',
basename='wire-element-example',
dependencies=[
'3rdparty:wire-runtime',
'examples/src/wire/org/pantsbuild/example/element',
'examples/src/wire/org/pantsbuild/example/temperature',
],
source='WireElementExample.java',
main='org.pantsbuild.example.wire.element.WireElementExample',
# TODO: The 'wire' library is not actually defined in a BUILD file: it comes in
# via the tool classpath of the sythetic target.
strict_deps=False,
)

Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@

package org.pantsbuild.example.wire.roots;

import org.pantsbuild.example.roots.Bar;

class WireRootsExample {

public static void main(String[] args) {
Bar bar = new Bar("one", "two");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,9 @@
jvm_binary(name='temperatureservice',
basename='wire-temperature-example',
dependencies=[
'3rdparty:wire-runtime',
'examples/src/wire/org/pantsbuild/example/temperature',
],
source='WireTemperatureExample.java',
main='org.pantsbuild.example.pants.temperatureservice.WireTemperatureExample',
# TODO: The 'wire' library is not actually defined in a BUILD file: it comes in
# via the tool classpath of the sythetic target.
strict_deps=False,
)
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

package org.pantsbuild.example

import org.pantsbuild.example.hello.welcome.WelcomeEverybody

// A simple jvm binary to test the jvm_run task on. Try, e.g.,
// ./pants -ldebug run --jvm-run-jvm-options='-Dfoo=bar' --jvm-run-jvm-program-args="Foo Bar" \\
// examples/src/scala/org/pantsbuild/example:jvm-run-example
Expand All @@ -11,6 +13,6 @@ package org.pantsbuild.example
object JvmRunExample {
def main(args: Array[String]) {
println("Hello, World")
println("args: " + args.mkString(", "))
println(WelcomeEverybody(args).mkString(", "))
}
}
2 changes: 1 addition & 1 deletion examples/src/thrift/org/pantsbuild/example/distance/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
java_thrift_library(name='distance-java',
sources=['distance.thrift'],
provides = artifact(org='org.pantsbuild.example',
name='distance-thrift-scala',
name='distance-thrift-java',
repo=public),
)

Expand Down
4 changes: 1 addition & 3 deletions examples/tests/java/org/pantsbuild/example/usewire/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@ junit_tests(name='usewire',
sources=['UseWireTest.java',],
dependencies=[
'3rdparty:junit',
'3rdparty:wire-runtime',
'examples/src/wire/org/pantsbuild/example/temperature',
],
# TODO: The 'wire' library is not actually defined in a BUILD file: it comes in
# via the tool classpath of the sythetic target.
strict_deps=False,
)
5 changes: 0 additions & 5 deletions src/python/pants/backend/codegen/tasks/antlr_gen.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,3 @@ def _scrub_generated_timestamps(self, target_workdir):
f.write(lines[0])
for line in lines[1:]:
f.write(line)

@property
def _copy_target_attributes(self):
"""Propagate the provides attribute to the synthetic java_library() target for publishing."""
return ['provides']
5 changes: 0 additions & 5 deletions src/python/pants/backend/codegen/tasks/apache_thrift_gen.py
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,3 @@ def _generate_thrift(self, target, target_workdir):
def execute_codegen(self, target, target_workdir):
self._validate(target)
self._generate_thrift(target, target_workdir)

@property
def _copy_target_attributes(self):
"""Propagate the provides attribute to the synthetic java_library() target for publishing."""
return ['provides']
5 changes: 0 additions & 5 deletions src/python/pants/backend/codegen/tasks/jaxb_gen.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,3 @@ def _correct_package(self, package):
if re.search(r'\.{2,}', package) is not None:
raise ValueError('Package name cannot have consecutive periods! ({})'.format(package))
return package

@property
def _copy_target_attributes(self):
"""Propagate the provides attribute to the synthetic java_library() target for publishing."""
return ['provides']
5 changes: 0 additions & 5 deletions src/python/pants/backend/codegen/tasks/protobuf_gen.py
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,3 @@ def _proto_path_imports(self, proto_targets):
for target in proto_targets:
for path in self._jars_to_directories(target):
yield os.path.relpath(path, get_buildroot())

@property
def _copy_target_attributes(self):
"""Propagate the provides attribute to the synthetic java_library() target for publishing."""
return ['provides']
5 changes: 0 additions & 5 deletions src/python/pants/backend/codegen/tasks/ragel_gen.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,6 @@ def execute_codegen(self, target, target_workdir):
raise TaskError('{binary} ... exited non-zero ({result})'
.format(binary=self.ragel_binary, result=result))

@property
def _copy_target_attributes(self):
"""Propagate the provides attribute to the synthetic java_library() target for publishing."""
return ['provides']


def calculate_class_and_package(path):
package, classname = None, None
Expand Down
7 changes: 5 additions & 2 deletions src/python/pants/backend/codegen/tasks/simple_codegen_task.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,11 @@ def execute(self):

@property
def _copy_target_attributes(self):
"""Return a list of attributes to be copied from the target to derived synthetic targets."""
return []
"""Return a list of attributes to be copied from the target to derived synthetic targets.
By default, propagates the provides, scope, and tags attributes.
"""
return ['provides', 'tags', 'scope']

def synthetic_target_dir(self, target, target_workdir):
"""
Expand Down
5 changes: 0 additions & 5 deletions src/python/pants/backend/codegen/tasks/wire_gen.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,3 @@ def collect_proto_paths(dep):
collect_proto_paths(target)
target.walk(collect_proto_paths)
return proto_paths

@property
def _copy_target_attributes(self):
"""Propagate the provides attribute to the synthetic java_library() target for publishing."""
return ['provides']
2 changes: 2 additions & 0 deletions src/python/pants/backend/jvm/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
from pants.backend.jvm.tasks.nailgun_task import NailgunKillall
from pants.backend.jvm.tasks.prepare_resources import PrepareResources
from pants.backend.jvm.tasks.prepare_services import PrepareServices
from pants.backend.jvm.tasks.provide_tools_jar import ProvideToolsJar
from pants.backend.jvm.tasks.run_jvm_prep_command import (RunBinaryJvmPrepCommand,
RunCompileJvmPrepCommand,
RunTestJvmPrepCommand)
Expand Down Expand Up @@ -135,6 +136,7 @@ def register_goals():
task(name='jvm-platform-validate', action=JvmPlatformValidate).install('jvm-platform-validate')

task(name='bootstrap-jvm-tools', action=BootstrapJvmTools).install('bootstrap')
task(name='provide-tools-jar', action=ProvideToolsJar).install('bootstrap')

# Compile
task(name='zinc', action=ZincCompile).install('compile')
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/backend/jvm/subsystems/scala_platform.py
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ def _library_target_spec(self, buildgraph, key, create_jardep_func):
synthetic_address = Address.parse('//:{}-synthetic'.format(key))
if not buildgraph.contains_address(synthetic_address):
jars = [create_jardep_func(self.version)]
buildgraph.inject_synthetic_target(synthetic_address, JarLibrary, jars=jars)
buildgraph.inject_synthetic_target(synthetic_address, JarLibrary, jars=jars, scope='forced')
elif not buildgraph.get_target(synthetic_address).is_synthetic:
raise buildgraph.ManualSyntheticTargetError(synthetic_address)
return buildgraph.get_target(synthetic_address).address.spec
4 changes: 3 additions & 1 deletion src/python/pants/backend/jvm/targets/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ python_library(
'jvm_prep_command.py',
'jvm_target.py',
'managed_jar_dependencies.py',
'tools_jar.py',
'unpacked_jars.py',
],
dependencies = [
Expand Down Expand Up @@ -59,9 +60,10 @@ python_library(
'java_tests.py',
],
dependencies = [
':jvm',
'3rdparty/python:six',
':jvm',
'src/python/pants/base:exceptions',
'src/python/pants/build_graph',
],
)

Expand Down
15 changes: 15 additions & 0 deletions src/python/pants/backend/jvm/targets/javac_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
unicode_literals, with_statement)

from pants.backend.jvm.targets.java_library import JavaLibrary
from pants.backend.jvm.targets.tools_jar import ToolsJar
from pants.build_graph.address import Address


class JavacPlugin(JavaLibrary):
Expand All @@ -24,3 +26,16 @@ def __init__(self, classname=None, plugin=None, *args, **kwargs):
self.plugin = plugin or self.name
self.classname = classname
self.add_labels('javac_plugin')

@property
def traversable_dependency_specs(self):
for spec in super(JavacPlugin, self).traversable_dependency_specs:
yield spec
yield self._tools_jar_spec(self._build_graph)

@classmethod
def _tools_jar_spec(cls, buildgraph):
synthetic_address = Address.parse('//:tools-jar-synthetic')
if not buildgraph.contains_address(synthetic_address):
buildgraph.inject_synthetic_target(synthetic_address, ToolsJar)
return synthetic_address.spec
5 changes: 4 additions & 1 deletion src/python/pants/backend/jvm/targets/scalac_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,11 @@
class ScalacPlugin(ScalaLibrary):
"""A Scala compiler plugin."""

def __init__(self, classname=None, plugin=None, *args, **kwargs):
@classmethod
def subsystem_dependencies(cls):
return super(ScalacPlugin, cls).subsystem_dependencies() + (ScalaPlatform,)

def __init__(self, classname=None, plugin=None, *args, **kwargs):
"""
:param classname: The fully qualified plugin class name - required.
:param plugin: The name of the plugin. Defaults to name if not supplied.
Expand Down
18 changes: 18 additions & 0 deletions src/python/pants/backend/jvm/targets/tools_jar.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# coding=utf-8
# Copyright 2016 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
unicode_literals, with_statement)

from pants.build_graph.target import Target


class ToolsJar(Target):
"""A private target type injected by the JavacPlugin to represent the JDK's tools.jar.
The classpath for this target is provided by the ProvideToolsJar task.
"""

def __init__(self, *args, **kwargs):
super(ToolsJar, self).__init__(scope='compile', *args, **kwargs)
Loading

0 comments on commit 5b7d317

Please sign in to comment.