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

implement exports literal in jvm_target #4329

Merged
merged 7 commits into from Mar 20, 2017
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 32 additions & 0 deletions src/python/pants/backend/jvm/targets/jvm_target.py
Expand Up @@ -13,8 +13,10 @@
from pants.backend.jvm.targets.jar_library import JarLibrary
from pants.backend.jvm.targets.jarable import Jarable
from pants.base.deprecated import deprecated_conditional
from pants.base.exceptions import TargetDefinitionException
from pants.base.payload import Payload
from pants.base.payload_field import ExcludesField, PrimitiveField, SetOfPrimitivesField
from pants.build_graph.address import Address
from pants.build_graph.resources import Resources
from pants.build_graph.target import Target
from pants.util.memo import memoized_property
Expand All @@ -40,6 +42,7 @@ def __init__(self,
services=None,
platform=None,
strict_deps=None,
exports=None,
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this could even go on Target itself? There's nothing intrinsically JVM-y about the concept, that I can see. The same is true for strict_deps itself, of course, but I wouldn't expect you to move that in this change.

Or is there some reason why it makes more sense for this to be only on JVM targets?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The drive for "exports" literal was a common issue in both javac and scalac. So far we have only seen the issue with java and scala targets. Also bazel only has this under java_library rule (not saying we should do whatever bazel does, but I feel limiting the usage of exports is reasonable).

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. It wouldn't be hard to expand later.

fatal_warnings=None,
zinc_file_manager=None,
# Some subclasses can have both .java and .scala sources
Expand Down Expand Up @@ -75,6 +78,14 @@ def __init__(self,
at compilation time. This enforces that all direct deps of the target
are declared, and can improve compilation speed due to smaller
classpaths. Transitive deps are always provided at runtime.
:param list exports: A list of exported targets, which will be accessible to dependents even
with strict_deps turned on. A common use case is for library targets to
to export dependencies that it knows its dependents will need. Then any
dependents of that library target will have access to those dependencies
even when strict_deps is True. Note: exports is transitive, which means
dependents have access to the closure of exports. An example will be that
if A exports B, and B exports C, then any targets that depends on A will
have access to both B and C.
:param bool fatal_warnings: Whether to turn warnings into errors for this target. If present,
takes priority over the language's fatal-warnings option.
:param bool zinc_file_manager: Whether to use zinc provided file manager that allows
Expand All @@ -98,6 +109,7 @@ def __init__(self,
'excludes': excludes,
'platform': PrimitiveField(platform),
'strict_deps': PrimitiveField(strict_deps),
'exports': SetOfPrimitivesField(exports),
'fatal_warnings': PrimitiveField(fatal_warnings),
'zinc_file_manager': PrimitiveField(zinc_file_manager),
'javac_plugins': SetOfPrimitivesField(javac_plugins),
Expand Down Expand Up @@ -125,6 +137,26 @@ def strict_deps(self):
"""
return self.payload.strict_deps

@property
def exports(self):
"""A list of exported targets, which will be accessible to dependents.

:return: See constructor.
:rtype: list
"""
exports = []
for spec in self.payload.exports:
addr = Address.parse(spec, relative_to=self.address.spec_path)
target = self._build_graph.get_target(addr)
if target not in self.dependencies:
# This means the exported target was not injected before "self",
# thus it's not a valid export.
raise TargetDefinitionException(self,
'Invalid exports: "{}" is not a dependency of {}'.format(spec, self))
exports.append(target)

return exports

@property
def fatal_warnings(self):
"""If set, overrides the platform's default fatal_warnings setting.
Expand Down
16 changes: 13 additions & 3 deletions src/python/pants/backend/jvm/tasks/jvm_compile/compile_context.py
Expand Up @@ -13,23 +13,33 @@
from pants.util.contextutil import open_zip


def _resolve_aliases(target):
def _resolve_strict_dependencies(target):
for declared in target.dependencies:
if type(declared) in (AliasTarget, Target):
# Is an alias. Recurse to expand.
for r in _resolve_aliases(declared):
for r in _resolve_strict_dependencies(declared):
yield r
else:
yield declared

for export in _resolve_exports(declared):
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A function named _resolve_aliases is a slightly unexpected place to find this logic, since it has nothing to do with aliases, although it's clearly the most straightforward way to implement this. Maybe clarify by renaming _resolve_aliases to _resolve_strict_dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

yield export


def _resolve_exports(target):
for export in getattr(target, 'exports', []):
for exp in _resolve_exports(export):
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So exports are transitive? That should be documented on the exports target attribute.

In fact, this whole functionality should be written up on the docsite.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I feel like they shouldn't be transitive. If the goal is to make the exports of a library explicit at the library's declaration, then I think exports being transitive is confusing.

For example, in order to find all the exported targets of t(exports=['a','b','c']), you'd need to look at a, b and c's declarations too, and their exported targets declarations if they exist. If exports are intransitive, then all the necessary information is in t(exports=['a','b','c']).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify between the two scenarios:

  1. If A exports B, B exports C, then C should be available for A at compile time, which is what this bazel discussion is about https://groups.google.com/d/msg/bazel-discuss/CRd1BbkHVUw/8Vi1gPAjIAAJ
  2. If A depends on B, B exports C, C exports D, is D available for A at compile time? (I am not too clear about this one)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah the bazel discussion actually means that it is transitively for 2), so it will definitely be transitive for 1)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. After reading that thread, I think I changed my mind. I hadn't thought it through.

  1. This is the case we're shooting for either way
A -d-> B
B -e-> C

classpath(A) => B, C
  1. This is the transitive case
A -d-> B
B -e-> C
C -e-> D
# should either
#  only include direct exports
classpath(A) => B, C
#  or, include transitive exports
classpath(A) => B, C, D
  • If we make exports transitive, strict_deps makes dependencies intransitive, but not exports. I think that's reasonable, in that strict_deps applies to dependencies not exports.
  • For the classfile parsing case, eg if D provides an annotation, will we get a warning/error if it is not on the classpath along with C? I think we might get an error.
  • For the inheritance case, making exports transitive means that if you inherit from C in A, that scalac's LUB check will be able to find D, which is an outcome we want.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Exports should be transitive. Sorry. I think I wasn't clear enough in that last comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it was my bad. I didn't realize you post the comment already until I post mine and refresh the page.

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify: If A has B in its dependencies and B has C in its exports then would we want C to be on A's classpath? Or does A have to have B in its exports? I guess the latter, otherwise anything depending on A won't get C on its classpath?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it ought to work like this: A has C on its compile classpath. Given an X depending on A, X won't have C on its compile classpath because A doesn't export B or C. If A exports B, then C will be on X's classpath.

A -d-> B
B -e-> C
X -d-> A

cp(A) == {B, C}
cp(X) == {A}

# if we add
A -e-> B
# then
cp(X) == {A, B, C}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benjyw A target's "exports" only has effect on dependents of the target, not on target itself. Nick's description is the exact behavior I implemented in this patch.

yield exp
yield export


def strict_dependencies(target, dep_context):
"""Compute the 'strict' compile target dependencies for this target.

Results the declared dependencies of a target after alias expansion, with the addition
of compiler plugins and their transitive deps, since compiletime is actually runtime for them.
"""
for declared in _resolve_aliases(target):
for declared in _resolve_strict_dependencies(target):
if isinstance(declared, dep_context.compiler_plugin_types):
for r in declared.closure(bfs=True, **dep_context.target_closure_kwargs):
yield r
Expand Down
@@ -0,0 +1,9 @@
// Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
// Licensed under the Apache License, Version 2.0 (see LICENSE).

package org.pantsbuild.testproject.exports;

public class A {
public void foo_a() {
}
}
@@ -0,0 +1,9 @@
// Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
// Licensed under the Apache License, Version 2.0 (see LICENSE).

package org.pantsbuild.testproject.exports;

public class B extends A {
public void foo_b() {
}
}
27 changes: 27 additions & 0 deletions testprojects/tests/java/org/pantsbuild/testproject/exports/BUILD
@@ -0,0 +1,27 @@
java_library(
name='A',
sources = ['A.java'],
)

java_library(
name='B',
sources = ['B.java'],
dependencies=[':A'],
exports=[':A'],
strict_deps=True,
)

java_library(
name='C',
sources = ['C.java'],
dependencies=[':B'],
exports=[':B'],
strict_deps=True,
)

java_library(
name='D',
sources = ['D.java'],
dependencies=[':C'],
strict_deps=True,
)
@@ -0,0 +1,9 @@
// Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
// Licensed under the Apache License, Version 2.0 (see LICENSE).

package org.pantsbuild.testproject.exports;

public class C extends B {
public void foo_c() {
}
}
11 changes: 11 additions & 0 deletions testprojects/tests/java/org/pantsbuild/testproject/exports/D.java
@@ -0,0 +1,11 @@
// Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
// Licensed under the Apache License, Version 2.0 (see LICENSE).

package org.pantsbuild.testproject.exports;

public class D {
public void foo_d() {
C c = new C();
c.foo_c();
}
}
@@ -0,0 +1,9 @@
// Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
// Licensed under the Apache License, Version 2.0 (see LICENSE).

package org.pantsbuild.testproject.exports;

class A {
def foo_a() {
}
}
@@ -0,0 +1,9 @@
// Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
// Licensed under the Apache License, Version 2.0 (see LICENSE).

package org.pantsbuild.testproject.exports;

class B extends A {
def foo_b() {
}
}
27 changes: 27 additions & 0 deletions testprojects/tests/scala/org/pantsbuild/testproject/exports/BUILD
@@ -0,0 +1,27 @@
scala_library(
name='A',
sources = ['A.scala'],
)

scala_library(
name='B',
sources = ['B.scala'],
dependencies=[':A'],
exports=[':A'],
strict_deps=True,
)

scala_library(
name='C',
sources = ['C.scala'],
dependencies=[':B'],
exports=[':B'],
strict_deps=True,
)

scala_library(
name='D',
sources = ['D.scala'],
dependencies=[':C'],
strict_deps=True,
)
@@ -0,0 +1,9 @@
// Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
// Licensed under the Apache License, Version 2.0 (see LICENSE).

package org.pantsbuild.testproject.exports;

class C extends B {
def foo_c() {
}
}
@@ -0,0 +1,11 @@
// Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
// Licensed under the Apache License, Version 2.0 (see LICENSE).

package org.pantsbuild.testproject.exports;

class D {
def foo_d() {
val c = new C();
c.foo_c();
}
}
@@ -0,0 +1,9 @@
// Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
// Licensed under the Apache License, Version 2.0 (see LICENSE).

package org.pantsbuild.testproject.non_exports;

class A {
def foo_a() {
}
}
@@ -0,0 +1,9 @@
// Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
// Licensed under the Apache License, Version 2.0 (see LICENSE).

package org.pantsbuild.testproject.non_exports;

class B extends A {
def foo_b() {
}
}
@@ -0,0 +1,18 @@
scala_library(
name='A',
sources = ['A.scala'],
)

scala_library(
name='B',
sources = ['B.scala'],
dependencies=[':A'],
strict_deps=True,
)

scala_library(
name='C',
sources = ['C.scala'],
dependencies=[':B'],
strict_deps=True,
)
@@ -0,0 +1,11 @@
// Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
// Licensed under the Apache License, Version 2.0 (see LICENSE).

package org.pantsbuild.testproject.non_exports;

class C {
def foo_c() {
val b = new B();
b.foo_b();
}
}
11 changes: 11 additions & 0 deletions tests/python/pants_test/backend/jvm/tasks/jvm_compile/BUILD
Expand Up @@ -42,6 +42,17 @@ python_tests(
tags={'integration'},
)

python_tests(
name='dep_exports_integration',
sources=['test_dep_exports_integration.py'],
dependencies = [
'src/python/pants/base:build_environment',
'tests/python/pants_test:int-test',
],
tags={'integration'},
timeout=300,
)

python_tests(
name='missing_dependency_finder',
sources=['test_missing_dependency_finder.py'],
Expand Down
@@ -0,0 +1,65 @@
# coding=utf-8
# Copyright 2017 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)

import os
import shutil

from pants.base.build_environment import get_buildroot
from pants_test.pants_run_integration_test import PantsRunIntegrationTest


class DepExportsIntegrationTest(PantsRunIntegrationTest):

SRC_PREFIX = 'testprojects/tests'
SRC_TYPES = ['java', 'scala']
SRC_PACKAGE = 'org/pantsbuild/testproject/exports'

@classmethod
def hermetic(cls):
return True

def test_compilation(self):
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth adding a test case demonstrating that these do indeed fail to compile without the exports mechanism. Otherwise some change to the test files might cause the compilations in this test to succeed for other reasons, and the test wouldn't be testing what we think it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a case where exports is needed but not added. Thanks for the suggestion!

for lang in self.SRC_TYPES:
path = os.path.join(self.SRC_PREFIX, lang, self.SRC_PACKAGE)
pants_run = self.run_pants(['list', '{}::'.format(path)])
self.assert_success(pants_run)
target_list = pants_run.stdout_data.strip().split('\n')
for target in target_list:
pants_run = self.run_pants(['compile', '--compile-scalafmt-skip', target])
self.assert_success(pants_run)

def modify_exports_and_compile(self, target, modify_file):
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than cloning the sources, you might consider just defining a target that is expected to fail to compile: just need to add it to the list of testprojects targets that are expected to fail here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cloned here because I want to modify the source file to test cache invalidation. I don't want to corrupt the repo, thus I copied it to a temp dir. But I will add a failure case. Benjy also mentioned it. Are you saying I don't have to run the failure test here but just add to the list you linked?

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I was saying is that if you want to create a testprojects test that fails to compile, you can do that. You just have to add it to the list of targets that are known to fail.

But if you're trying to test invalidation, you do need to create the clone, so carry on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a fail target, and added it to the list you linked.
Those targets are skipped during integration, so I added another test in test_dep_exports_integration.py to make sure it fails.

with self.temporary_sourcedir() as tmp_src:
src_dir = os.path.relpath(os.path.join(tmp_src, os.path.basename(self.SRC_PACKAGE)), get_buildroot())
target_dir, target_name = target.rsplit(':', 1)
shutil.copytree(target_dir, src_dir)
with self.temporary_workdir() as workdir:
cmd = ['compile', '--compile-scalafmt-skip', '{}:{}'.format(src_dir, target_name)]
pants_run = self.run_pants_with_workdir(command=cmd, workdir=workdir)
self.assert_success(pants_run)

with open(os.path.join(src_dir, modify_file), 'ab') as fh:
fh.write('\n')

pants_run = self.run_pants_with_workdir(command=cmd, workdir=workdir)
self.assert_success(pants_run)
self.assertTrue('{}:{}'.format(src_dir, target_name) in pants_run.stdout_data)

def test_invalidation(self):
for lang in self.SRC_TYPES:
path = os.path.join(self.SRC_PREFIX, lang, self.SRC_PACKAGE)
target = '{}:D'.format(path)
self.modify_exports_and_compile(target, 'A.{}'.format(lang))
self.modify_exports_and_compile(target, 'B.{}'.format(lang))

def test_non_exports(self):
pants_run = self.run_pants(['compile', '--compile-scalafmt-skip',
'testprojects/tests/scala/org/pantsbuild/testproject/non_exports:C'])
self.assert_failure(pants_run)
self.assertIn('FAILURE: Compilation failure: Failed jobs: '
'compile(testprojects/tests/scala/org/pantsbuild/testproject/non_exports:C)',
pants_run.stdout_data)
Expand Up @@ -57,6 +57,7 @@ def targets(self):
'testprojects/tests/java/org/pantsbuild/testproject/empty:',
'testprojects/tests/java/org/pantsbuild/testproject/fail256:fail256',
'testprojects/tests/python/pants/dummies:failing_target',
'testprojects/tests/scala/org/pantsbuild/testproject/non_exports:C',
# These don't pass without special config.
'testprojects/tests/java/org/pantsbuild/testproject/depman:new-tests',
'testprojects/tests/java/org/pantsbuild/testproject/depman:old-tests',
Expand Down