From 3df7c004fd4a7ac6b5cf1eb697a58f05345fecd4 Mon Sep 17 00:00:00 2001 From: Patrick Lawson Date: Sat, 21 Aug 2021 18:16:26 -0400 Subject: [PATCH] Consume CoarsenedTargets (added in #12251) in javac for cycle resolution, including cycles created by subtargets. (#12231) Consume CoarsenedTargets (added in #12251) in javac for cycle resolution, including cycles created by subtargets. --- .../pants/backend/java/compile/javac.py | 89 ++-- .../pants/backend/java/compile/javac_test.py | 391 +++++++++++++++++- 2 files changed, 423 insertions(+), 57 deletions(-) diff --git a/src/python/pants/backend/java/compile/javac.py b/src/python/pants/backend/java/compile/javac.py index e63c2b7b453..3139f154ec4 100644 --- a/src/python/pants/backend/java/compile/javac.py +++ b/src/python/pants/backend/java/compile/javac.py @@ -5,14 +5,16 @@ 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, @@ -20,32 +22,14 @@ 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): - 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) @@ -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) ) + 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, @@ -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 + ), ) ), ) @@ -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, ), ) diff --git a/src/python/pants/backend/java/compile/javac_test.py b/src/python/pants/backend/java/compile/javac_test.py index cf9511bdf86..680c7f3cd2c 100644 --- a/src/python/pants/backend/java/compile/javac_test.py +++ b/src/python/pants/backend/java/compile/javac_test.py @@ -14,8 +14,10 @@ from pants.build_graph.address import Address from pants.core.util_rules import config_files, source_files from pants.core.util_rules.external_tool import rules as external_tool_rules +from pants.engine.addresses import Addresses from pants.engine.fs import DigestContents, FileDigest from pants.engine.internals.scheduler import ExecutionError +from pants.engine.target import CoarsenedTarget, CoarsenedTargets, Targets from pants.jvm.resolve.coursier_fetch import ( CoursierLockfileEntry, CoursierResolvedLockfile, @@ -45,6 +47,7 @@ def rule_runner() -> RuleRunner: *util_rules(), *javac_binary_rules(), QueryRule(CompiledClassfiles, (CompileJavaSourceRequest,)), + QueryRule(CoarsenedTargets, (Addresses,)), ], target_types=[JvmDependencyLockfile, JavaLibrary], ) @@ -77,6 +80,17 @@ def rule_runner() -> RuleRunner: ) +def expect_single_expanded_coarsened_target( + rule_runner: RuleRunner, address: Address +) -> CoarsenedTarget: + expanded_target = rule_runner.request(Targets, [Addresses([address])]).expect_single() + coarsened_targets = rule_runner.request( + CoarsenedTargets, [Addresses([expanded_target.address])] + ) + assert len(coarsened_targets) == 1 + return coarsened_targets[0] + + def test_compile_no_deps(rule_runner: RuleRunner) -> None: rule_runner.write_files( { @@ -109,13 +123,17 @@ def test_compile_no_deps(rule_runner: RuleRunner) -> None: CompiledClassfiles, [ CompileJavaSourceRequest( - target=rule_runner.get_target(address=Address(spec_path="", target_name="lib")) + component=expect_single_expanded_coarsened_target( + rule_runner, Address(spec_path="", target_name="lib") + ) ) ], ) + classfile_digest_contents = rule_runner.request(DigestContents, [compiled_classfiles.digest]) - assert len(classfile_digest_contents) == 1 - assert classfile_digest_contents[0].path == "org/pantsbuild/example/lib/ExampleLib.class" + assert frozenset(content.path for content in classfile_digest_contents) == frozenset( + ["org/pantsbuild/example/lib/ExampleLib.class"] + ) def test_compile_jdk_versions(rule_runner: RuleRunner) -> None: @@ -145,10 +163,12 @@ def test_compile_jdk_versions(rule_runner: RuleRunner) -> None: "ExampleLib.java": JAVA_LIB_SOURCE, } ) + request = CompileJavaSourceRequest( - target=rule_runner.get_target(address=Address(spec_path="", target_name="lib")) + component=expect_single_expanded_coarsened_target( + rule_runner, Address(spec_path="", target_name="lib") + ) ) - rule_runner.set_options(["--javac-jdk=zulu:1.6"]) assert { contents.path @@ -163,6 +183,332 @@ def test_compile_jdk_versions(rule_runner: RuleRunner) -> None: rule_runner.request(CompiledClassfiles, [request]) +def test_compile_multiple_source_files(rule_runner: RuleRunner) -> None: + rule_runner.write_files( + { + "BUILD": dedent( + """\ + coursier_lockfile( + name = 'lockfile', + maven_requirements = [], + sources = [ + "coursier_resolve.lockfile", + ], + ) + + java_library( + name = 'lib', + dependencies = [ + ':lockfile', + ] + ) + """ + ), + "coursier_resolve.lockfile": CoursierResolvedLockfile(entries=()) + .to_json() + .decode("utf-8"), + "ExampleLib.java": JAVA_LIB_SOURCE, + "OtherLib.java": dedent( + """\ + package org.pantsbuild.example.lib; + + public class OtherLib { + public static String hello() { + return "Hello!"; + } + } + """ + ), + } + ) + + expanded_targets = rule_runner.request( + Targets, [Addresses([Address(spec_path="", target_name="lib")])] + ) + assert sorted(t.address.spec for t in expanded_targets) == [ + "//ExampleLib.java:lib", + "//OtherLib.java:lib", + ] + + coarsened_targets = rule_runner.request( + CoarsenedTargets, [Addresses([t.address for t in expanded_targets])] + ) + assert len(coarsened_targets) == 1 + coarsened_target = coarsened_targets[0] + assert len(coarsened_target.members) == 2 + request = CompileJavaSourceRequest(component=coarsened_target) + + compiled_classfiles = rule_runner.request(CompiledClassfiles, [request]) + classfile_digest_contents = rule_runner.request(DigestContents, [compiled_classfiles.digest]) + assert frozenset(content.path for content in classfile_digest_contents) == frozenset( + ["org/pantsbuild/example/lib/ExampleLib.class", "org/pantsbuild/example/lib/OtherLib.class"] + ) + + +def test_compile_with_cycle(rule_runner: RuleRunner) -> None: + """Test that javac can handle source-level cycles--even across build target boundaries--via + graph coarsening. + + This test has to set up a contrived dependency since build-target cycles are forbidden by the graph. However, + file-target cycles are not forbidden, so we configure the graph like so: + + a:a has a single source file, which has file-target address a/A.java, and which inherits a:a's + explicit dependency on b/B.java. + b:b depends directly on a:a, and its source b/B.java inherits that dependency. + + Therefore, after target expansion via Get(Targets, Addresses(...)), we get the cycle of: + + a/A.java -> b/B.java -> a/A.java + """ + + rule_runner.write_files( + { + "BUILD": dedent( + """\ + coursier_lockfile( + name = 'lockfile', + maven_requirements = [], + sources = [ + "coursier_resolve.lockfile", + ], + ) + """ + ), + "coursier_resolve.lockfile": CoursierResolvedLockfile(entries=()) + .to_json() + .decode("utf-8"), + "a/BUILD": dedent( + """\ + java_library( + name = 'a', + dependencies = [ + '//:lockfile', + 'b/B.java', + ] + ) + """ + ), + "a/A.java": dedent( + """\ + package org.pantsbuild.a; + import org.pantsbuild.b.B; + public interface A {} + class C implements B {} + """ + ), + "b/BUILD": dedent( + """\ + java_library( + name = 'b', + dependencies = [ + '//:lockfile', + 'a/A.java', + ] + ) + """ + ), + "b/B.java": dedent( + """\ + package org.pantsbuild.b; + import org.pantsbuild.a.A; + public interface B {}; + class C implements A {} + """ + ), + } + ) + coarsened_target = expect_single_expanded_coarsened_target( + rule_runner, Address(spec_path="a", target_name="a") + ) + assert sorted([t.address.spec for t in coarsened_target.members]) == ["a/A.java", "b/B.java"] + request = CompileJavaSourceRequest(component=coarsened_target) + + compiled_classfiles = rule_runner.request(CompiledClassfiles, [request]) + classfile_digest_contents = rule_runner.request(DigestContents, [compiled_classfiles.digest]) + assert frozenset(content.path for content in classfile_digest_contents) == frozenset( + [ + "org/pantsbuild/a/A.class", + "org/pantsbuild/a/C.class", + "org/pantsbuild/b/B.class", + "org/pantsbuild/b/C.class", + ] + ) + + +def test_compile_with_transitive_cycle(rule_runner: RuleRunner) -> None: + """Like test_compile_with_cycle, but the cycle occurs as a transitive dep of the requested + target.""" + + rule_runner.write_files( + { + "BUILD": dedent( + """\ + coursier_lockfile( + name = 'lockfile', + maven_requirements = [], + sources = [ + "coursier_resolve.lockfile", + ], + ) + + java_library( + name = 'main', + dependencies = [ + '//:lockfile', + 'a:a', + ] + ) + """ + ), + "Main.java": dedent( + """\ + package org.pantsbuild.main; + import org.pantsbuild.a.A; + public class Main implements A {} + """ + ), + "coursier_resolve.lockfile": CoursierResolvedLockfile(entries=()) + .to_json() + .decode("utf-8"), + "a/BUILD": dedent( + """\ + java_library( + name = 'a', + dependencies = [ + '//:lockfile', + 'b/B.java', + ] + ) + """ + ), + "a/A.java": dedent( + """\ + package org.pantsbuild.a; + import org.pantsbuild.b.B; + public interface A {} + class C implements B {} + """ + ), + "b/BUILD": dedent( + """\ + java_library( + name = 'b', + dependencies = [ + '//:lockfile', + 'a:a', + ] + ) + """ + ), + "b/B.java": dedent( + """\ + package org.pantsbuild.b; + import org.pantsbuild.a.A; + public interface B {}; + class C implements A {} + """ + ), + } + ) + + compiled_classfiles = rule_runner.request( + CompiledClassfiles, + [ + CompileJavaSourceRequest( + component=expect_single_expanded_coarsened_target( + rule_runner, Address(spec_path="", target_name="main") + ) + ) + ], + ) + classfile_digest_contents = rule_runner.request(DigestContents, [compiled_classfiles.digest]) + assert frozenset(content.path for content in classfile_digest_contents) == frozenset( + ["org/pantsbuild/main/Main.class"] + ) + + +def test_compile_with_transitive_multiple_sources(rule_runner: RuleRunner) -> None: + """Like test_compile_with_transitive_cycle, but the cycle occurs via subtarget source expansion + rather than explicitly.""" + + rule_runner.write_files( + { + "BUILD": dedent( + """\ + coursier_lockfile( + name = 'lockfile', + maven_requirements = [], + sources = [ + "coursier_resolve.lockfile", + ], + ) + + java_library( + name = 'main', + dependencies = [ + '//:lockfile', + 'lib:lib', + ] + ) + """ + ), + "Main.java": dedent( + """\ + package org.pantsbuild.main; + import org.pantsbuild.a.A; + import org.pantsbuild.b.B; + public class Main implements A {} + class Other implements B {} + """ + ), + "coursier_resolve.lockfile": CoursierResolvedLockfile(entries=()) + .to_json() + .decode("utf-8"), + "lib/BUILD": dedent( + """\ + java_library( + name = 'lib', + dependencies = [ + '//:lockfile', + ] + ) + """ + ), + "lib/A.java": dedent( + """\ + package org.pantsbuild.a; + import org.pantsbuild.b.B; + public interface A {} + class C implements B {} + """ + ), + "lib/B.java": dedent( + """\ + package org.pantsbuild.b; + import org.pantsbuild.a.A; + public interface B {}; + class C implements A {} + """ + ), + } + ) + + compiled_classfiles = rule_runner.request( + CompiledClassfiles, + [ + CompileJavaSourceRequest( + component=expect_single_expanded_coarsened_target( + rule_runner, Address(spec_path="", target_name="main") + ) + ) + ], + ) + classfile_digest_contents = rule_runner.request(DigestContents, [compiled_classfiles.digest]) + assert frozenset(content.path for content in classfile_digest_contents) == frozenset( + ["org/pantsbuild/main/Main.class", "org/pantsbuild/main/Other.class"] + ) + + def test_compile_with_deps(rule_runner: RuleRunner) -> None: rule_runner.write_files( { @@ -202,12 +548,13 @@ def test_compile_with_deps(rule_runner: RuleRunner) -> None: "lib/ExampleLib.java": JAVA_LIB_SOURCE, } ) - compiled_classfiles = rule_runner.request( CompiledClassfiles, [ CompileJavaSourceRequest( - target=rule_runner.get_target(address=Address(spec_path="", target_name="main")) + component=expect_single_expanded_coarsened_target( + rule_runner, Address(spec_path="", target_name="main") + ) ) ], ) @@ -243,13 +590,14 @@ def test_compile_with_missing_dep_fails(rule_runner: RuleRunner) -> None: .decode("utf-8"), } ) - - compile_request = CompileJavaSourceRequest( - target=rule_runner.get_target(address=Address(spec_path="", target_name="main")) + request = CompileJavaSourceRequest( + component=expect_single_expanded_coarsened_target( + rule_runner, Address(spec_path="", target_name="main") + ) ) expected_exception_msg = r".*?package org.pantsbuild.example.lib does not exist.*?" with pytest.raises(ExecutionError, match=expected_exception_msg): - rule_runner.request(CompiledClassfiles, [compile_request]) + rule_runner.request(CompiledClassfiles, [request]) def test_compile_with_maven_deps(rule_runner: RuleRunner) -> None: @@ -304,15 +652,12 @@ def test_compile_with_maven_deps(rule_runner: RuleRunner) -> None: ), } ) - - compiled_classfiles = rule_runner.request( - CompiledClassfiles, - [ - CompileJavaSourceRequest( - target=rule_runner.get_target(address=Address(spec_path="", target_name="main")) - ) - ], + request = CompileJavaSourceRequest( + component=expect_single_expanded_coarsened_target( + rule_runner, Address(spec_path="", target_name="main") + ) ) + compiled_classfiles = rule_runner.request(CompiledClassfiles, [request]) classfile_digest_contents = rule_runner.request(DigestContents, [compiled_classfiles.digest]) assert len(classfile_digest_contents) == 1 assert classfile_digest_contents[0].path == "org/pantsbuild/example/Example.class" @@ -359,9 +704,11 @@ def test_compile_with_missing_maven_dep_fails(rule_runner: RuleRunner) -> None: } ) - compile_request = CompileJavaSourceRequest( - target=rule_runner.get_target(address=Address(spec_path="", target_name="main")) + request = CompileJavaSourceRequest( + component=expect_single_expanded_coarsened_target( + rule_runner, Address(spec_path="", target_name="main") + ) ) expected_exception_msg = r".*?package org.joda.time does not exist.*?" with pytest.raises(ExecutionError, match=expected_exception_msg): - rule_runner.request(CompiledClassfiles, [compile_request]) + rule_runner.request(CompiledClassfiles, [request])