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.

Sign up for GitHub

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

Consume CoarsenedTargets (added in #12251) in javac for cycle resolution, including cycles created by subtargets. #12231

Merged
merged 2 commits into from
Aug 21, 2021
Merged

Consume CoarsenedTargets (added in #12251) in javac for cycle resolution, including cycles created by subtargets. #12231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea0ca37
Consume CoarsenedTargets in javac for cycle resolution
patricklaw Jul 4, 2021
cc4e6a0
Uncomment test skip, to be resolved in #12425
patricklaw Aug 21, 2021
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
89 changes: 54 additions & 35 deletions src/python/pants/backend/java/compile/javac.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,47 +5,31 @@

import logging
from dataclasses import dataclass
from itertools import chain

from pants.backend.java.compile.javac_binary import JavacBinary
from pants.backend.java.target_types import JavaSources
from pants.core.util_rules.source_files import SourceFiles, SourceFilesRequest
from pants.engine.addresses import Addresses
from pants.engine.fs import EMPTY_DIGEST, AddPrefix, Digest, MergeDigests, RemovePrefix
from pants.engine.process import BashBinary, Process, ProcessResult
from pants.engine.rules import Get, MultiGet, collect_rules, rule
from pants.engine.target import Dependencies, DependenciesRequest, Sources, Target, Targets
from pants.engine.target import CoarsenedTarget, CoarsenedTargets, Sources, Targets
from pants.jvm.resolve.coursier_fetch import (
CoursierLockfileForTargetRequest,
CoursierResolvedLockfile,
MaterializedClasspath,
MaterializedClasspathRequest,
)
from pants.jvm.resolve.coursier_setup import Coursier
from pants.option.subsystem import Subsystem
from pants.util.logging import LogLevel

logger = logging.getLogger(__name__)


class JavacSubsystem(Subsystem):
patricklaw marked this conversation as resolved.
Show resolved Hide resolved
options_scope = "javac"
help = "The javac Java source compiler."

@classmethod
def register_options(cls, register):
super().register_options(register)
register(
"--jdk",
default="adopt:1.11",
advanced=True,
help="The JDK to use for invoking javac."
" This string will be passed directly to Coursier's `--jvm` parameter."
" Run `cs java --available` to see a list of available JVM versions on your platform.",
)


@dataclass(frozen=True)
class CompileJavaSourceRequest:
target: Target
component: CoarsenedTarget


@dataclass(frozen=True)
Expand All @@ -60,26 +44,53 @@ async def compile_java_source(
javac_binary: JavacBinary,
request: CompileJavaSourceRequest,
) -> CompiledClassfiles:
sources = await Get(
SourceFiles,
SourceFilesRequest(
(request.target.get(Sources),),
for_sources_types=(JavaSources,),
enable_codegen=True,
component_members_with_sources = tuple(
t for t in request.component.members if t.has_field(Sources)
)
component_members_and_source_files = zip(
component_members_with_sources,
await MultiGet(
Get(
SourceFiles,
SourceFilesRequest(
(t.get(Sources),),
for_sources_types=(JavaSources,),
enable_codegen=True,
),
)
for t in component_members_with_sources
),
)
if sources.snapshot.digest == EMPTY_DIGEST:

component_members_and_java_source_files = [
(target, sources)
for target, sources in component_members_and_source_files
if sources.snapshot.digest != EMPTY_DIGEST
]

if not component_members_and_java_source_files:
return CompiledClassfiles(digest=EMPTY_DIGEST)

lockfile, direct_deps = await MultiGet(
Get(CoursierResolvedLockfile, CoursierLockfileForTargetRequest(Targets((request.target,)))),
Get(Targets, DependenciesRequest(request.target.get(Dependencies))),
# Target coarsening currently doesn't perform dep expansion, which matters for targets
# with multiple sources that expand to individual source subtargets.
# We expand the dependencies explicitly here before coarsening, but ideally this could
# be done somehow during coarsening.
# TODO: Should component dependencies be filtered out here if they were only brought in by component members which were
# filtered out above (due to having no JavaSources to contribute)? If so, that will likely required extending
# the CoarsenedTargets API to include more complete dependency information, or to support such filtering directly.
expanded_direct_deps = await Get(Targets, Addresses(request.component.dependencies))
coarsened_direct_deps = await Get(
CoarsenedTargets, Addresses(t.address for t in expanded_direct_deps)
)
Comment on lines +74 to 84
Copy link
Member

Choose a reason for hiding this comment

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

Hm. I don't think that this should be necessary.

How I had expected that this would work was that if you received a compilation request for a base target (i.e., one with no sources), then you would coarsen it and recursively request compilation of its dependencies, which would be the expanded/file level targets... which would actually have sources (and so on).

Currently it looks like you're bailing early if a target has no sources, but I would expect the recursion to continue: if you're compiling recursively, some nodes in the recursion not having sources shouldn't short-circuit the recursion.

Copy link
Member

@stuhood stuhood Jul 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

You can see how the graph ends up being complete in the unit tests: the CoarsenedTargets for the base targets (dep, t1, and t2) depend on their own files.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I had a fair amount of trouble deciding on an overall mental model here. Consuming a CoarsenedTarget in CompileJavaSourceRequest was where I landed. I also waffled for quite a while on spliting this into two rules: one for dealing with expanding and coarsening targets, and one lower level rule for actually running the compilation against a set of JavaSources fields.

I'm open to changing this, but let me explain my thought process:

if you're compiling recursively, some nodes in the recursion not having sources shouldn't short-circuit the recursion.

The whole crux of the problem with javac is limiting my dependency classpath to only the current compilation unit's direct dependencies. We don't want the full transitive closure on the classpath of the current compilation unit, by design, even if we might indirectly, recursively build it in order to build our direct dependencies. So we're really distinguishing here between our compile-time (direct dep) classpath, and a "runtime" classpath that is likely the closure's classpath. This is why the recursion eagerly terminates when it concludes that the current component doesn't produce any classpath entries: we don't need to build it, which means we don't need its deps (only used to build it), which means we don't recursively compile its deps.

But by direct dependency, we really mean direct source dependency, or possibly some other kind of resource dependency that can somehow contribute to the classpath but isn't yet modeled. This is where things get sticky, because target expansion sort of changes the definition of "direct" dependency.

You can see how the graph ends up being complete in the unit tests: the CoarsenedTargets for the base targets (dep, t1, and t2) depend on their own files.

That test only uses explicit file dependencies; it doesn't cover the case I'm covering here where, e.g. t1 depends on dep instead of dep.txt:dep.

An example:

A.java -> B -- (expands to) --> (B.java and B2.java).

If we use target expansion, we'll automatically get A.java -> B.java; A.java -> C.java; and B.java <--> C.java. So subtarget expansion ends up getting us our natural notion of direct dependencies. Just coarsening B will not do this; we need to do both. See javac_test.py:test_compile_with_transitive_multiple_sources in this PR for a concrete example of this.

Stepping way back: my mental model here was to operate over the nodes of an induced coarsened, expanded graph. Then each rule invocation maps 1:1 with a coarsened, expanded component, and its only job is to make sure that its recursive calls are also over coarsened, expanded components. Expanding gets us the right notion of direct dependencies in the presence of subtarget expansion, while coarsening gets us the right notion of a single compilation unit in the presence of source-target cycles.

I'm pretty confident this needs a more generic model, since we're going to need to eventually worry about other rules that can contribute to the compilation classpath, which means we're probably going to need to request a union product from our dependencies. That's where I was imagining two layers of rules, where compile_java_source is lower level and not expected to think hard about how to expand its dependencies. But without another example of a rule that produces files for the compilation classpath, I worry that attempting to model that is premature. So I opted for the "direct" route here just to get javac working and exercising the existing coarsening/expansion machinery.

Again, I'm happy to change this if you have an idea of how to remodel it, but pay particular attention to the example in javac_test.py:test_compile_with_transitive_multiple_sources to make sure that that case is covered (or why that case is somehow not actually a valid graph to test against).


lockfile = await Get(
CoursierResolvedLockfile,
CoursierLockfileForTargetRequest(Targets(request.component.members)),
)
direct_dependency_classfiles = await MultiGet(
Get(CompiledClassfiles, CompileJavaSourceRequest(target=target)) for target in direct_deps
Get(CompiledClassfiles, CompileJavaSourceRequest(component=coarsened_dep))
for coarsened_dep in coarsened_direct_deps
)

materialized_classpath, merged_direct_dependency_classfiles_digest = await MultiGet(
Get(
MaterializedClasspath,
Expand All @@ -105,10 +116,13 @@ async def compile_java_source(
Digest,
MergeDigests(
(
sources.snapshot.digest,
prefixed_direct_dependency_classfiles_digest,
materialized_classpath.digest,
javac_binary.digest,
*(
sources.snapshot.digest
for _, sources in component_members_and_java_source_files
),
)
),
)
Expand All @@ -123,11 +137,16 @@ async def compile_java_source(
classpath_arg,
"-d",
"classfiles",
*sources.files,
*sorted(
chain.from_iterable(
sources.snapshot.files
for _, sources in component_members_and_java_source_files
)
),
],
input_digest=merged_digest,
output_directories=("classfiles",),
description=f"Compile {request.target.address.spec} with javac",
description=f"Compile {request.component.members} with javac",
level=LogLevel.DEBUG,
),
)
Expand Down
Loading