Skip to content

Commit

Permalink
[jvm] Apply the resolve for deploy_jar and restore validation of re…
Browse files Browse the repository at this point in the history
…solve compatibility (pantsbuild#13911)

The `resolve` field for `deploy_jar` was not being respected, because the `deploy_jar` itself was not included in the targets for which we were calculating the resolve (only its dependencies). To fix this, we add support to `ClasspathEntryRequest` for "root only" requests, which allow for requesting a classpath for a target when it is a root, but failing when the target type is used as an inner node (since we don't want `deploy_jar`s to be used as dependencies of JVM compile targets anytime soon).

Additionally, restore the validation of resolve compatibility when computing a `CoursierResolveKey` (temporarily removed in pantsbuild#13876). This time around, the `@rule` takes `CoarsenedTargets` as input, which removes redundant walking while checking the transitive graph, while still allowing us to differentiate between the roots of the request, and the dependencies of the request.

[ci skip-rust]
[ci skip-build-wheels]
  • Loading branch information
stuhood authored Dec 17, 2021
1 parent 287a5a2 commit 86fe4e0
Show file tree
Hide file tree
Showing 10 changed files with 284 additions and 66 deletions.
6 changes: 4 additions & 2 deletions src/python/pants/backend/java/goals/check.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from pants.core.goals.check import CheckRequest, CheckResult, CheckResults
from pants.engine.addresses import Addresses
from pants.engine.rules import Get, MultiGet, collect_rules, rule
from pants.engine.target import CoarsenedTargets, Targets
from pants.engine.target import CoarsenedTargets
from pants.engine.unions import UnionMembership, UnionRule
from pants.jvm.compile import ClasspathEntryRequest, FallibleClasspathEntry
from pants.jvm.resolve.key import CoursierResolveKey
Expand All @@ -31,8 +31,10 @@ async def javac_check(
CoarsenedTargets, Addresses(field_set.address for field_set in request.field_sets)
)

# NB: Each root can have an independent resolve, because there is no inherent relation
# between them other than that they were on the commandline together.
resolves = await MultiGet(
Get(CoursierResolveKey, Targets(t.members)) for t in coarsened_targets
Get(CoursierResolveKey, CoarsenedTargets([t])) for t in coarsened_targets
)

results = await MultiGet(
Expand Down
70 changes: 62 additions & 8 deletions src/python/pants/backend/java/package/deploy_jar.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,20 @@
PackageFieldSet,
)
from pants.core.util_rules.archive import ZipBinary
from pants.engine.fs import AddPrefix, CreateDigest, Digest, FileContent, MergeDigests
from pants.engine.addresses import Addresses
from pants.engine.fs import EMPTY_DIGEST, AddPrefix, CreateDigest, Digest, FileContent, MergeDigests
from pants.engine.process import BashBinary, Process, ProcessResult
from pants.engine.rules import Get, collect_rules, rule
from pants.engine.target import Dependencies, DependenciesRequest
from pants.engine.unions import UnionRule
from pants.engine.rules import Get, MultiGet, collect_rules, rule
from pants.engine.target import Dependencies
from pants.engine.unions import UnionMembership, UnionRule
from pants.jvm import classpath
from pants.jvm.classpath import Classpath
from pants.jvm.compile import (
ClasspathEntry,
ClasspathEntryRequest,
CompileResult,
FallibleClasspathEntry,
)

logger = logging.getLogger(__name__)

Expand All @@ -43,15 +50,61 @@ class DeployJarFieldSet(PackageFieldSet):
dependencies: Dependencies


class DeployJarClasspathEntryRequest(ClasspathEntryRequest):
field_sets = (DeployJarFieldSet,)
# A `deploy_jar` can have a Classpath requested for it, but should not be used as a dependency.
root_only = True


@rule
async def deploy_jar_classpath(
request: DeployJarClasspathEntryRequest,
union_membership: UnionMembership,
) -> FallibleClasspathEntry:
if len(request.component.members) > 1:
# If multiple DeployJar targets were coarsened into a single instance, it's because they
# formed a cycle among themselves... but at a high level, they shouldn't have dependencies
# on one another anyway.
raise Exception(
"`deploy_jar` targets should not depend on one another:\n"
f"{request.component.bullet_list()}"
)
classpath_entries = FallibleClasspathEntry.if_all_succeeded(
await MultiGet(
Get(
FallibleClasspathEntry,
ClasspathEntryRequest,
ClasspathEntryRequest.for_targets(
union_membership, component=coarsened_dep, resolve=request.resolve
),
)
for coarsened_dep in request.component.dependencies
)
)
if classpath_entries is None:
return FallibleClasspathEntry(
description=str(request.component),
result=CompileResult.DEPENDENCY_FAILED,
output=None,
exit_code=1,
)
return FallibleClasspathEntry(
description=str(request.component),
result=CompileResult.SUCCEEDED,
output=ClasspathEntry(EMPTY_DIGEST, dependencies=classpath_entries),
exit_code=0,
)


@rule
async def package_deploy_jar(
bash: BashBinary,
zip: ZipBinary,
field_set: DeployJarFieldSet,
) -> BuiltPackage:
"""
Constructs a deploy ("fat") JAR file (currently from Java sources only) by
1. Resolving/compiling a classpath for the `root_address` target,
Constructs a deploy ("fat") JAR file by
1. Resolving/compiling a Classpath for the `root_address` target,
2. Producing a ZIP file containing _only_ the JAR manifest file for the `main_class`
3. Creating a deploy jar with a broken ZIP index by concatenating all dependency JARs together,
followed by the thin JAR we created
Expand All @@ -62,10 +115,10 @@ async def package_deploy_jar(
raise Exception("Needs a `main` argument")

#
# 1. Produce a thin JAR containing our first-party sources and other runtime dependencies
# 1. Produce thin JARs containing the transitive classpath
#

classpath = await Get(Classpath, DependenciesRequest(field_set.dependencies))
classpath = await Get(Classpath, Addresses([field_set.address]))

#
# 2. Produce JAR manifest, and output to a ZIP file that can be included with the JARs
Expand Down Expand Up @@ -164,4 +217,5 @@ def rules():
*collect_rules(),
*classpath.rules(),
UnionRule(PackageFieldSet, DeployJarFieldSet),
UnionRule(ClasspathEntryRequest, DeployJarClasspathEntryRequest),
]
6 changes: 4 additions & 2 deletions src/python/pants/backend/scala/goals/check.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from pants.core.goals.check import CheckRequest, CheckResult, CheckResults
from pants.engine.addresses import Addresses
from pants.engine.rules import Get, MultiGet, collect_rules, rule
from pants.engine.target import CoarsenedTargets, Targets
from pants.engine.target import CoarsenedTargets
from pants.engine.unions import UnionMembership, UnionRule
from pants.jvm.compile import ClasspathEntryRequest, FallibleClasspathEntry
from pants.jvm.resolve.key import CoursierResolveKey
Expand All @@ -31,8 +31,10 @@ async def scalac_check(
CoarsenedTargets, Addresses(field_set.address for field_set in request.field_sets)
)

# NB: Each root can have an independent resolve, because there is no inherent relation
# between them other than that they were on the commandline together.
resolves = await MultiGet(
Get(CoursierResolveKey, Targets(t.members)) for t in coarsened_targets
Get(CoursierResolveKey, CoarsenedTargets([t])) for t in coarsened_targets
)

results = await MultiGet(
Expand Down
10 changes: 1 addition & 9 deletions src/python/pants/backend/scala/goals/repl.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,13 @@
# Licensed under the Apache License, Version 2.0 (see LICENSE).
from __future__ import annotations

import itertools

from pants.backend.scala.compile.scala_subsystem import ScalaSubsystem
from pants.core.goals.repl import ReplImplementation, ReplRequest
from pants.engine.addresses import Addresses
from pants.engine.fs import AddPrefix, Digest, MergeDigests
from pants.engine.internals.selectors import Get, MultiGet
from pants.engine.process import BashBinary
from pants.engine.rules import collect_rules, rule
from pants.engine.target import Dependencies, DependenciesRequest
from pants.engine.unions import UnionRule
from pants.jvm.classpath import Classpath
from pants.jvm.jdk_rules import JdkSetup
Expand All @@ -32,13 +29,8 @@ class ScalaRepl(ReplImplementation):
async def create_scala_repl_request(
repl: ScalaRepl, bash: BashBinary, jdk_setup: JdkSetup, scala_subsystem: ScalaSubsystem
) -> ReplRequest:
dependencies_for_each_target = await MultiGet(
Get(Addresses, DependenciesRequest(tgt[Dependencies])) for tgt in repl.targets
)
dependencies = list(itertools.chain.from_iterable(dependencies_for_each_target))

user_classpath, tool_classpath = await MultiGet(
Get(Classpath, Addresses(dependencies)),
Get(Classpath, Addresses(t.address for t in repl.targets)),
Get(
MaterializedClasspath,
MaterializedClasspathRequest(
Expand Down
6 changes: 5 additions & 1 deletion src/python/pants/engine/target.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
from pants.util.memo import memoized_classproperty, memoized_method, memoized_property
from pants.util.meta import frozen_after_init
from pants.util.ordered_set import FrozenOrderedSet
from pants.util.strutil import pluralize
from pants.util.strutil import bullet_list, pluralize

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -673,6 +673,10 @@ def representative(self) -> Target:
"""A stable "representative" target in the cycle."""
return next(iter(self.members))

def bullet_list(self) -> str:
"""The addresses and type aliases of all members of the cycle."""
return bullet_list(sorted(f"{t.address.spec}\t({type(t).alias})" for t in self.members))

def __hash__(self) -> int:
return self._hashcode

Expand Down
13 changes: 8 additions & 5 deletions src/python/pants/jvm/classpath.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from pants.engine.collection import Collection
from pants.engine.fs import Digest
from pants.engine.rules import Get, MultiGet, collect_rules, rule
from pants.engine.target import CoarsenedTargets, Targets
from pants.engine.target import CoarsenedTargets
from pants.engine.unions import UnionMembership
from pants.jvm.compile import ClasspathEntry, ClasspathEntryRequest
from pants.jvm.resolve.key import CoursierResolveKey
Expand Down Expand Up @@ -65,15 +65,18 @@ async def classpath(
coarsened_targets: CoarsenedTargets,
union_membership: UnionMembership,
) -> Classpath:
resolve = await Get(
CoursierResolveKey, Targets(t for ct in coarsened_targets.closure() for t in ct.members)
)
# Compute a single shared resolve for all of the roots, which will validate that they
# are compatible with one another.
resolve = await Get(CoursierResolveKey, CoarsenedTargets, coarsened_targets)

# Then request classpath entries for each root.
classpath_entries = await MultiGet(
Get(
ClasspathEntry,
ClasspathEntryRequest,
ClasspathEntryRequest.for_targets(union_membership, component=t, resolve=resolve),
ClasspathEntryRequest.for_targets(
union_membership, component=t, resolve=resolve, root=True
),
)
for t in coarsened_targets
)
Expand Down
32 changes: 25 additions & 7 deletions src/python/pants/jvm/compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ class ClasspathSourceAmbiguity(Exception):
"""Too many compiler instances were compatible with a CoarsenedTarget."""


class ClasspathRootOnlyWasInner(Exception):
"""A root_only request type was used as an inner node in a compile graph."""


class _ClasspathEntryRequestClassification(Enum):
COMPATIBLE = auto()
PARTIAL = auto()
Expand Down Expand Up @@ -66,11 +70,23 @@ class ClasspathEntryRequest(metaclass=ABCMeta):
# `cls.field_sets`.
field_sets_consume_only: ClassVar[tuple[type[FieldSet], ...]] = ()

# True if this request type is only valid at the root of a compile graph.
root_only: ClassVar[bool] = False

@staticmethod
def for_targets(
union_membership: UnionMembership, component: CoarsenedTarget, resolve: CoursierResolveKey
union_membership: UnionMembership,
component: CoarsenedTarget,
resolve: CoursierResolveKey,
*,
root: bool = False,
) -> ClasspathEntryRequest:
"""Constructs a subclass compatible with the members of the CoarsenedTarget."""
"""Constructs a subclass compatible with the members of the CoarsenedTarget.
If the CoarsenedTarget is a root of a compile graph, pass `root=True` to allow usage of
request types which are marked `root_only`.
"""

compatible = []
partial = []
consume_only = []
Expand All @@ -87,6 +103,11 @@ def for_targets(
consume_only.append(impl)

if len(compatible) == 1:
if not root and impl.root_only:
raise ClasspathRootOnlyWasInner(
"The following targets had dependees, but can only be used as roots in a "
f"build graph:\n{component.bullet_list()}"
)
return compatible[0](component, resolve, None)

# No single request can handle the entire component: see whether there are exactly one
Expand All @@ -97,21 +118,18 @@ def for_targets(
return partial[0](component, resolve, consume_only[0](component, resolve, None))

impls_str = ", ".join(sorted(impl.__name__ for impl in impls))
targets_str = "\n ".join(
sorted(f"{t.address.spec}\t({type(t).alias})" for t in component.members)
)
if compatible:
raise ClasspathSourceAmbiguity(
f"More than one JVM classpath provider ({impls_str}) was compatible with "
f"the inputs:\n {targets_str}"
f"the inputs:\n{component.bullet_list()}"
)
else:
# TODO: There is more subtlety of error messages possible here if there are multiple
# partial providers, but can cross that bridge when we have them (multiple Scala or Java
# compiler implementations, for example).
raise ClasspathSourceMissing(
f"No JVM classpath providers (from: {impls_str}) were compatible with the "
f"combination of inputs:\n {targets_str}"
f"combination of inputs:\n{component.bullet_list()}"
)

@staticmethod
Expand Down
Loading

0 comments on commit 86fe4e0

Please sign in to comment.