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

[internal] Make JvmLockfileRequest generic #14201

Merged
merged 2 commits into from
Jan 19, 2022
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/python/pants/backend/codegen/avro/java/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
)
from pants.engine.unions import UnionRule
from pants.jvm.goals import lockfile
from pants.jvm.goals.lockfile import JvmLockfileRequest
from pants.jvm.goals.lockfile import JvmLockfileRequest, JvmLockfileRequestFromTool
from pants.jvm.jdk_rules import JdkSetup
from pants.jvm.resolve.coursier_fetch import MaterializedClasspath, MaterializedClasspathRequest
from pants.source.source_root import SourceRoot, SourceRootRequest
Expand Down Expand Up @@ -211,10 +211,9 @@ def make_avro_process(

@rule
async def generate_avro_tools_lockfile_request(
_: AvroToolLockfileSentinel,
tool: AvroSubsystem,
_: AvroToolLockfileSentinel, tool: AvroSubsystem
) -> JvmLockfileRequest:
return JvmLockfileRequest.from_tool(tool)
return await Get(JvmLockfileRequest, JvmLockfileRequestFromTool(tool))


def rules():
Expand Down
7 changes: 3 additions & 4 deletions src/python/pants/backend/codegen/protobuf/scala/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
from pants.engine.unions import UnionRule
from pants.jvm.compile import ClasspathEntry
from pants.jvm.goals import lockfile
from pants.jvm.goals.lockfile import JvmLockfileRequest
from pants.jvm.goals.lockfile import JvmLockfileRequest, JvmLockfileRequestFromTool
from pants.jvm.jdk_rules import JdkSetup
from pants.jvm.resolve.common import ArtifactRequirements, Coordinate
from pants.jvm.resolve.coursier_fetch import MaterializedClasspath, MaterializedClasspathRequest
Expand Down Expand Up @@ -368,10 +368,9 @@ async def setup_scalapb_shim_classfiles(

@rule
async def generate_scalapbc_lockfile_request(
_: ScalapbcToolLockfileSentinel,
tool: ScalaPBSubsystem,
_: ScalapbcToolLockfileSentinel, tool: ScalaPBSubsystem
) -> JvmLockfileRequest:
return JvmLockfileRequest.from_tool(tool)
return await Get(JvmLockfileRequest, JvmLockfileRequestFromTool(tool))


def rules():
Expand Down
7 changes: 3 additions & 4 deletions src/python/pants/backend/codegen/thrift/scrooge/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
from pants.engine.target import TransitiveTargets, TransitiveTargetsRequest, WrappedTarget
from pants.engine.unions import UnionRule
from pants.jvm.goals import lockfile
from pants.jvm.goals.lockfile import JvmLockfileRequest
from pants.jvm.goals.lockfile import JvmLockfileRequest, JvmLockfileRequestFromTool
from pants.jvm.jdk_rules import JdkSetup
from pants.jvm.resolve.coursier_fetch import MaterializedClasspath, MaterializedClasspathRequest
from pants.source.source_root import SourceRootsRequest, SourceRootsResult
Expand Down Expand Up @@ -135,10 +135,9 @@ async def generate_scrooge_thrift_sources(

@rule
async def generate_scrooge_lockfile_request(
_: ScroogeToolLockfileSentinel,
scrooge: ScroogeSubsystem,
_: ScroogeToolLockfileSentinel, scrooge: ScroogeSubsystem
) -> JvmLockfileRequest:
return JvmLockfileRequest.from_tool(scrooge)
return await Get(JvmLockfileRequest, JvmLockfileRequestFromTool(scrooge))


def rules():
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from pants.engine.target import FieldSet, Target
from pants.engine.unions import UnionRule
from pants.jvm.goals import lockfile
from pants.jvm.goals.lockfile import JvmLockfileRequest
from pants.jvm.goals.lockfile import JvmLockfileRequest, JvmLockfileRequestFromTool
from pants.jvm.jdk_rules import JdkSetup
from pants.jvm.resolve.coursier_fetch import MaterializedClasspath, MaterializedClasspathRequest
from pants.util.logging import LogLevel
Expand Down Expand Up @@ -158,10 +158,9 @@ async def google_java_format_lint(

@rule
async def generate_google_java_format_lockfile_request(
_: GoogleJavaFormatToolLockfileSentinel,
tool: GoogleJavaFormatSubsystem,
_: GoogleJavaFormatToolLockfileSentinel, tool: GoogleJavaFormatSubsystem
) -> JvmLockfileRequest:
return JvmLockfileRequest.from_tool(tool)
return await Get(JvmLockfileRequest, JvmLockfileRequestFromTool(tool))


def rules():
Expand Down
15 changes: 12 additions & 3 deletions src/python/pants/backend/scala/compile/scalac_plugins.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,13 @@
from pants.engine.unions import UnionRule
from pants.jvm.goals import lockfile
from pants.jvm.goals.lockfile import JvmLockfileRequest
from pants.jvm.resolve.common import ArtifactRequirements
from pants.jvm.resolve.coursier_fetch import (
CoursierResolvedLockfile,
MaterializedClasspath,
MaterializedClasspathRequest,
)
from pants.jvm.resolve.jvm_tool import GatherJvmCoordinatesRequest
from pants.jvm.resolve.jvm_tool import rules as jvm_tool_rules
from pants.util.ordered_set import FrozenOrderedSet
from pants.util.strutil import bullet_list
Expand Down Expand Up @@ -55,7 +57,7 @@ async def parse_global_scalac_plugins(scalac_plugins: Scalac) -> _LoadedGlobalSc

if invalid_targets:
raise ValueError(
f"The `[{Scalac.options_scope}].global` option accepts only "
f"The `[{Scalac.options_scope}].plugins_global` option accepts only "
f"`{ScalacPluginTarget.alias}` targets, but got:\n\n"
f"{bullet_list(type(t).alias for t in invalid_targets)}"
)
Expand All @@ -70,13 +72,20 @@ class GlobalScalacPluginsToolLockfileSentinel(ToolLockfileSentinel):


@rule
def generate_global_scalac_plugins_lockfile_request(
async def generate_global_scalac_plugins_lockfile_request(
_: GlobalScalacPluginsToolLockfileSentinel,
loaded_global_plugins: _LoadedGlobalScalacPlugins,
scalac_plugins: Scalac,
) -> JvmLockfileRequest:
artifacts = await Get(
ArtifactRequirements,
GatherJvmCoordinatesRequest(
FrozenOrderedSet(loaded_global_plugins.artifact_address_inputs),
f"[{scalac_plugins.options_scope}].plugins_global",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm starting to see a lot of GatherJvmCoordinatesRequests with that f-string, is there an opportunity to create a util here to make sure these requests are more easily reused?

),
)
return JvmLockfileRequest(
artifact_inputs=FrozenOrderedSet(loaded_global_plugins.artifact_address_inputs),
artifacts=artifacts,
resolve_name="scalac-plugins",
lockfile_dest=scalac_plugins.plugins_global_lockfile,
)
Expand Down
9 changes: 6 additions & 3 deletions src/python/pants/backend/scala/compile/scalac_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,8 @@ def test_compile_with_scalac_plugin(rule_runner: RuleRunner) -> None:

scalac_plugin(
name = "acyclic",
artifact = ":acyclic_lib",
# TODO: Support relative addresses.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was always broken, but now we're more eagerly validating the address so it makes a difference. I can try fixing in a followup? We need to use AddressInput.parse(relative_to)

artifact = "lib:acyclic_lib",
)

scala_sources(
Expand Down Expand Up @@ -468,7 +469,8 @@ def test_compile_with_multiple_scalac_plugins(rule_runner: RuleRunner) -> None:
scalac_plugin(
name="kind-projector",
plugin_name="kind-projector",
artifact=":kind-projector-lib",
# TODO: Support relative addresses.
artifact="lib:kind-projector-lib",
)

jvm_artifact(
Expand All @@ -481,7 +483,8 @@ def test_compile_with_multiple_scalac_plugins(rule_runner: RuleRunner) -> None:
scalac_plugin(
name="better-monadic-for",
plugin_name="bm4",
artifact=":better-monadic-for-lib",
# TODO: Support relative addresses.
artifact="lib:better-monadic-for-lib",
)
"""
),
Expand Down
7 changes: 3 additions & 4 deletions src/python/pants/backend/scala/lint/scalafmt/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
from pants.engine.target import FieldSet, Target
from pants.engine.unions import UnionRule
from pants.jvm.goals import lockfile
from pants.jvm.goals.lockfile import JvmLockfileRequest
from pants.jvm.goals.lockfile import JvmLockfileRequest, JvmLockfileRequestFromTool
from pants.jvm.jdk_rules import JdkSetup
from pants.jvm.resolve.coursier_fetch import MaterializedClasspath, MaterializedClasspathRequest
from pants.util.frozendict import FrozenDict
Expand Down Expand Up @@ -318,10 +318,9 @@ async def scalafmt_lint(field_sets: ScalafmtRequest, tool: ScalafmtSubsystem) ->

@rule
async def generate_scalafmt_lockfile_request(
_: ScalafmtToolLockfileSentinel,
tool: ScalafmtSubsystem,
_: ScalafmtToolLockfileSentinel, tool: ScalafmtSubsystem
) -> JvmLockfileRequest:
return JvmLockfileRequest.from_tool(tool)
return await Get(JvmLockfileRequest, JvmLockfileRequestFromTool(tool))


def rules():
Expand Down
4 changes: 2 additions & 2 deletions src/python/pants/backend/scala/test/scalatest.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
from pants.engine.unions import UnionRule
from pants.jvm.classpath import Classpath
from pants.jvm.goals import lockfile
from pants.jvm.goals.lockfile import JvmLockfileRequest
from pants.jvm.goals.lockfile import JvmLockfileRequest, JvmLockfileRequestFromTool
from pants.jvm.jdk_rules import JdkSetup
from pants.jvm.resolve.coursier_fetch import MaterializedClasspath, MaterializedClasspathRequest
from pants.jvm.subsystems import JvmSubsystem
Expand Down Expand Up @@ -164,7 +164,7 @@ async def setup_scalatest_debug_request(field_set: ScalatestTestFieldSet) -> Tes
async def generate_scalatest_lockfile_request(
_: ScalatestToolLockfileSentinel, scalatest: Scalatest
) -> JvmLockfileRequest:
return JvmLockfileRequest.from_tool(scalatest)
return await Get(JvmLockfileRequest, JvmLockfileRequestFromTool(scalatest))


def rules():
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/init/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,4 @@ target(
],
)

python_tests(name="tests", timeout=200)
python_tests(name="tests", timeout=240)
49 changes: 35 additions & 14 deletions src/python/pants/jvm/goals/lockfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,45 @@
from pants.jvm.resolve.jvm_tool import GatherJvmCoordinatesRequest, JvmToolBase
from pants.jvm.resolve.key import CoursierResolveKey
from pants.util.logging import LogLevel
from pants.util.meta import frozen_after_init
from pants.util.ordered_set import FrozenOrderedSet


@dataclass(frozen=True)
class JvmLockfileRequest(LockfileRequest):
artifacts: ArtifactRequirements


@frozen_after_init
@dataclass(unsafe_hash=True)
class JvmLockfileRequestFromTool:
artifact_inputs: FrozenOrderedSet[str]
options_scope: str
lockfile_dest: str

@classmethod
def from_tool(cls, tool: JvmToolBase) -> JvmLockfileRequest:
return cls(
artifact_inputs=FrozenOrderedSet(tool.artifact_inputs),
resolve_name=tool.options_scope,
lockfile_dest=tool.lockfile,
)
def __init__(self, tool: JvmToolBase) -> None:
# Note that `JvmToolBase` is not hashable, so we extract the relevant information eagerly.
self.artifact_inputs = FrozenOrderedSet(tool.artifact_inputs)
self.options_scope = tool.options_scope
self.lockfile_dest = tool.lockfile


@rule
async def setup_lockfile_request_from_tool(
request: JvmLockfileRequestFromTool,
) -> JvmLockfileRequest:
artifacts = await Get(
ArtifactRequirements,
GatherJvmCoordinatesRequest(
request.artifact_inputs,
f"[{request.options_scope}].artifacts",
),
)
return JvmLockfileRequest(
artifacts=artifacts,
resolve_name=request.options_scope,
lockfile_dest=request.lockfile_dest,
)


@rule
Expand All @@ -45,11 +70,7 @@ def wrap_python_lockfile_request(request: JvmLockfileRequest) -> WrappedLockfile
async def generate_jvm_lockfile(
request: JvmLockfileRequest,
) -> Lockfile:
requirements = await Get(
ArtifactRequirements,
GatherJvmCoordinatesRequest(request.artifact_inputs, f"[{request.resolve_name}].artifacts"),
)
resolved_lockfile = await Get(CoursierResolvedLockfile, ArtifactRequirements, requirements)
resolved_lockfile = await Get(CoursierResolvedLockfile, ArtifactRequirements, request.artifacts)
lockfile_digest = await Get(
Digest,
CreateDigest([FileContent(request.lockfile_dest, resolved_lockfile.to_serialized())]),
Expand All @@ -62,13 +83,13 @@ async def load_jvm_lockfile(
request: JvmLockfileRequest,
) -> CoursierResolvedLockfile:
"""Loads an existing lockfile from disk."""
if not request.artifact_inputs:
if not request.artifacts:
return CoursierResolvedLockfile(entries=())

lockfile_snapshot = await Get(Snapshot, PathGlobs([request.lockfile_dest]))
if not lockfile_snapshot.files:
raise ValueError(
f"JVM tool `{request.resolve_name}` does not have a lockfile generated. "
f"JVM resolve `{request.resolve_name}` does not have a lockfile generated. "
f"Run `{GenerateLockfilesSubsystem.name} --resolve={request.resolve_name} to "
"generate it."
)
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/jvm/resolve/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def __init__(self, coords: str) -> None:
super().__init__(f"Received invalid artifact coordinates: {coords}")


@dataclass(frozen=True)
@dataclass(frozen=True, order=True)
class Coordinate:
"""A single Maven-style coordinate for a JVM dependency.

Expand Down
25 changes: 9 additions & 16 deletions src/python/pants/jvm/resolve/jvm_tool_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,14 @@
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.fs import Digest, DigestContents
from pants.engine.rules import SubsystemRule, rule
from pants.jvm.goals.lockfile import JvmLockfileRequest
from pants.engine.rules import Get, SubsystemRule, rule
from pants.jvm.goals.lockfile import JvmLockfileRequest, JvmLockfileRequestFromTool
from pants.jvm.goals.lockfile import rules as lockfile_rules
from pants.jvm.resolve import jvm_tool
from pants.jvm.resolve.common import ArtifactRequirements, Coordinate
from pants.jvm.resolve.common import Coordinate
from pants.jvm.resolve.coursier_fetch import rules as coursier_fetch_rules
from pants.jvm.resolve.coursier_setup import rules as coursier_setup_rules
from pants.jvm.resolve.jvm_tool import GatherJvmCoordinatesRequest, JvmToolBase
from pants.jvm.resolve.jvm_tool import JvmToolBase
from pants.jvm.target_types import JvmArtifactTarget
from pants.jvm.util_rules import rules as util_rules
from pants.testutil.rule_runner import PYTHON_BOOTSTRAP_ENV, QueryRule, RuleRunner
Expand All @@ -39,7 +40,7 @@ class MockJvmToolLockfileSentinel(ToolLockfileSentinel):
async def generate_test_tool_lockfile_request(
_: MockJvmToolLockfileSentinel, tool: MockJvmTool
) -> JvmLockfileRequest:
return JvmLockfileRequest.from_tool(tool)
return await Get(JvmLockfileRequest, JvmLockfileRequestFromTool(tool))


def test_jvm_tool_base_extracts_correct_coordinates() -> None:
Expand All @@ -52,10 +53,10 @@ def test_jvm_tool_base_extracts_correct_coordinates() -> None:
*source_files.rules(),
*util_rules(),
*jvm_tool.rules(),
*lockfile_rules(),
generate_test_tool_lockfile_request,
SubsystemRule(MockJvmTool),
QueryRule(JvmLockfileRequest, (MockJvmToolLockfileSentinel,)),
QueryRule(ArtifactRequirements, (GatherJvmCoordinatesRequest,)),
QueryRule(DigestContents, (Digest,)),
],
target_types=[JvmArtifactTarget],
Expand Down Expand Up @@ -83,16 +84,8 @@ def test_jvm_tool_base_extracts_correct_coordinates() -> None:
}
)
lockfile_request = rule_runner.request(JvmLockfileRequest, [MockJvmToolLockfileSentinel()])
assert sorted(lockfile_request.artifact_inputs) == [
"//:junit_junit",
Copy link
Contributor

Choose a reason for hiding this comment

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

One side-effect of this assert here is that we validated that both Maven-style and Address-style artifact specifications were tests. Are we doing this anywhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry, I had on auto-merge. Will make sure the followup PR has tests for this.

"org.hamcrest:hamcrest-core:1.3",
]

requirements = rule_runner.request(
ArtifactRequirements, [GatherJvmCoordinatesRequest(lockfile_request.artifact_inputs, "")]
)
coordinates = [i.coordinate for i in requirements]
assert sorted(coordinates, key=lambda c: (c.group, c.artifact, c.version)) == [
coordinates = sorted(i.coordinate for i in lockfile_request.artifacts)
assert coordinates == [
Coordinate(group="junit", artifact="junit", version="4.13.2"),
Coordinate(group="org.hamcrest", artifact="hamcrest-core", version="1.3"),
]
4 changes: 2 additions & 2 deletions src/python/pants/jvm/test/junit.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
from pants.engine.unions import UnionRule
from pants.jvm.classpath import Classpath
from pants.jvm.goals import lockfile
from pants.jvm.goals.lockfile import JvmLockfileRequest
from pants.jvm.goals.lockfile import JvmLockfileRequest, JvmLockfileRequestFromTool
from pants.jvm.jdk_rules import JdkSetup
from pants.jvm.resolve.coursier_fetch import MaterializedClasspath, MaterializedClasspathRequest
from pants.jvm.subsystems import JvmSubsystem
Expand Down Expand Up @@ -161,7 +161,7 @@ async def setup_junit_debug_request(field_set: JunitTestFieldSet) -> TestDebugRe
async def generate_junit_lockfile_request(
_: JunitToolLockfileSentinel, junit: JUnit
) -> JvmLockfileRequest:
return JvmLockfileRequest.from_tool(junit)
return await Get(JvmLockfileRequest, JvmLockfileRequestFromTool(junit))


def rules():
Expand Down