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

Fix lockfile generation for duplicate jvm_artifact targets with jar fields. #15219

Merged
merged 4 commits into from
Apr 22, 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
8 changes: 4 additions & 4 deletions src/python/pants/backend/scala/resolve/lockfile_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
from pants.engine.internals import build_files, graph
from pants.jvm import jdk_rules
from pants.jvm.goals import lockfile
from pants.jvm.goals.lockfile import GenerateJvmLockfile, RequestedJVMserResolveNames
from pants.jvm.goals.lockfile import GenerateJvmLockfile, RequestedJVMUserResolveNames
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 rules as coursier_jvm_tool_rules
Expand Down Expand Up @@ -44,7 +44,7 @@ def rule_runner() -> RuleRunner:
*graph.rules(),
*build_files.rules(),
*target_types.rules(),
QueryRule(UserGenerateLockfiles, (RequestedJVMserResolveNames,)),
QueryRule(UserGenerateLockfiles, (RequestedJVMUserResolveNames,)),
QueryRule(GenerateLockfileResult, (GenerateJvmLockfile,)),
],
target_types=[JvmArtifactTarget, ScalaSourceTarget, ScalaSourcesGeneratorTarget],
Expand All @@ -71,7 +71,7 @@ def test_missing_scala_library_triggers_error(rule_runner: RuleRunner) -> None:
with engine_error(ValueError, contains="does not contain a requirement for the Scala runtime"):
_ = rule_runner.request(
UserGenerateLockfiles,
[RequestedJVMserResolveNames(["foo"])],
[RequestedJVMUserResolveNames(["foo"])],
)


Expand Down Expand Up @@ -101,5 +101,5 @@ def test_conflicting_scala_library_triggers_error(rule_runner: RuleRunner) -> No
):
_ = rule_runner.request(
UserGenerateLockfiles,
[RequestedJVMserResolveNames(["foo"])],
[RequestedJVMUserResolveNames(["foo"])],
)
16 changes: 9 additions & 7 deletions src/python/pants/jvm/goals/lockfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

from collections import defaultdict
from dataclasses import dataclass
from typing import Mapping

from pants.core.goals.generate_lockfiles import (
GenerateLockfile,
Expand All @@ -28,6 +29,7 @@
from pants.jvm.target_types import JvmArtifactResolveField, JvmResolveField
from pants.util.docutil import bin_name
from pants.util.logging import LogLevel
from pants.util.ordered_set import OrderedSet


@dataclass(frozen=True)
Expand Down Expand Up @@ -79,7 +81,7 @@ async def generate_jvm_lockfile(
return GenerateLockfileResult(lockfile_digest, request.resolve_name, request.lockfile_dest)


class RequestedJVMserResolveNames(RequestedUserResolveNames):
class RequestedJVMUserResolveNames(RequestedUserResolveNames):
pass


Expand All @@ -94,7 +96,7 @@ def determine_jvm_user_resolves(
return KnownUserResolveNames(
names=tuple(jvm_subsystem.resolves.keys()),
option_name=f"[{jvm_subsystem.options_scope}].resolves",
requested_resolve_names_cls=RequestedJVMserResolveNames,
requested_resolve_names_cls=RequestedJVMUserResolveNames,
)


Expand Down Expand Up @@ -128,12 +130,12 @@ async def validate_jvm_artifacts_for_resolve(

@rule
async def setup_user_lockfile_requests(
requested: RequestedJVMserResolveNames,
requested: RequestedJVMUserResolveNames,
all_targets: AllTargets,
jvm_subsystem: JvmSubsystem,
) -> UserGenerateLockfiles:
resolve_to_artifacts = defaultdict(set)
for tgt in all_targets:
resolve_to_artifacts: Mapping[str, OrderedSet[ArtifactRequirement]] = defaultdict(OrderedSet)
for tgt in sorted(all_targets, key=lambda t: t.address):
if not tgt.has_field(JvmArtifactResolveField):
continue
artifact = ArtifactRequirement.from_jvm_artifact_target(tgt)
Expand All @@ -146,7 +148,7 @@ async def setup_user_lockfile_requests(
Get(
GenerateJvmLockfile,
_ValidateJvmArtifactsRequest(
artifacts=ArtifactRequirements(sorted(resolve_to_artifacts.get(resolve, ()))),
artifacts=ArtifactRequirements(resolve_to_artifacts.get(resolve, ())),
resolve_name=resolve,
),
)
Expand All @@ -162,5 +164,5 @@ def rules():
*coursier_fetch.rules(),
UnionRule(GenerateLockfile, GenerateJvmLockfile),
UnionRule(KnownUserResolveNamesRequest, KnownJVMUserResolveNamesRequest),
UnionRule(RequestedUserResolveNames, RequestedJVMserResolveNames),
UnionRule(RequestedUserResolveNames, RequestedJVMUserResolveNames),
)
53 changes: 39 additions & 14 deletions src/python/pants/jvm/goals/lockfile_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,17 @@
from __future__ import annotations

from textwrap import dedent
from typing import cast

import pytest

from pants.core.goals.generate_lockfiles import GenerateLockfileResult, UserGenerateLockfiles
from pants.core.util_rules import source_files
from pants.core.util_rules.external_tool import rules as external_tool_rules
from pants.engine.fs import DigestContents, FileDigest
from pants.engine.internals.parametrize import Parametrize
from pants.jvm.goals import lockfile
from pants.jvm.goals.lockfile import GenerateJvmLockfile, RequestedJVMserResolveNames
from pants.jvm.goals.lockfile import GenerateJvmLockfile, RequestedJVMUserResolveNames
from pants.jvm.resolve.common import (
ArtifactRequirement,
ArtifactRequirements,
Expand All @@ -39,10 +41,11 @@ def rule_runner() -> RuleRunner:
*external_tool_rules(),
*source_files.rules(),
*util_rules(),
QueryRule(UserGenerateLockfiles, [RequestedJVMserResolveNames]),
QueryRule(UserGenerateLockfiles, [RequestedJVMUserResolveNames]),
QueryRule(GenerateLockfileResult, [GenerateJvmLockfile]),
],
target_types=[JvmArtifactTarget],
objects={"parametrize": Parametrize},
)
rule_runner.set_options([], env_inherit={"PATH"})
return rule_runner
Expand Down Expand Up @@ -82,27 +85,49 @@ def test_generate_lockfile(rule_runner: RuleRunner) -> None:
assert CoursierResolvedLockfile.from_serialized(digest_contents[0].content) == expected


@maybe_skip_jdk_test
def test_artifact_collision(rule_runner: RuleRunner) -> None:
# Test that an artifact with fully populated but identical fields can be generated.
rule_runner.write_files(
{
"BUILD": dedent(
"""\
def mk(name):
jvm_artifact(
name=name,
group='group',
artifact='artifact',
version='1',
jar='jar.jar',
excludes=['ex:clude'],
)

mk('one')
mk('two')
"""
),
}
)

result = rule_runner.request(
UserGenerateLockfiles, [RequestedJVMUserResolveNames(["jvm-default"])]
)
# Because each instance of the jar field is unique.
assert len(cast(GenerateJvmLockfile, result[0]).artifacts) == 2


@maybe_skip_jdk_test
def test_multiple_resolves(rule_runner: RuleRunner) -> None:
# TODO: Adjust to use https://github.com/pantsbuild/pants/pull/14408 for the
# duplicated artifact.
rule_runner.write_files(
{
"BUILD": dedent(
"""\
jvm_artifact(
name='hamcrest_a',
name='hamcrest',
group='org.hamcrest',
artifact='hamcrest-core',
version="1.3",
resolve="a",
)
jvm_artifact(
name='hamcrest_b',
group='org.hamcrest',
artifact='hamcrest-core',
version="1.3",
resolve="b",
resolve=parametrize("a", "b"),
)

jvm_artifact(
Expand All @@ -126,7 +151,7 @@ def test_multiple_resolves(rule_runner: RuleRunner) -> None:
)
rule_runner.set_options(["--jvm-resolves={'a': 'a.lock', 'b': 'b.lock'}"], env_inherit={"PATH"})

result = rule_runner.request(UserGenerateLockfiles, [RequestedJVMserResolveNames(["a", "b"])])
result = rule_runner.request(UserGenerateLockfiles, [RequestedJVMUserResolveNames(["a", "b"])])
hamcrest_core = ArtifactRequirement(Coordinate("org.hamcrest", "hamcrest-core", "1.3"))
assert set(result) == {
GenerateJvmLockfile(
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 @@ -142,7 +142,7 @@ class Coordinates(DeduplicatedCollection[Coordinate]):
"""An ordered list of `Coordinate`s."""


@dataclass(frozen=True, order=True)
@dataclass(frozen=True)
class ArtifactRequirement:
"""A single Maven-style coordinate for a JVM dependency, along with information of how to fetch
the dependency if it is not to be fetched from a Maven repository."""
Expand Down