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

Adds excludes field to jvm_artifact targets #14715

Merged
Merged
Show file tree
Hide file tree
Changes from 3 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
31 changes: 24 additions & 7 deletions src/python/pants/jvm/resolve/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

from __future__ import annotations

import dataclasses
import re
from dataclasses import dataclass
from typing import Iterable
Expand All @@ -12,6 +13,7 @@
from pants.engine.target import Target
from pants.jvm.target_types import (
JvmArtifactArtifactField,
JvmArtifactExcludeDependenciesField,
JvmArtifactFieldSet,
JvmArtifactGroupField,
JvmArtifactJarSourceField,
Expand Down Expand Up @@ -149,6 +151,7 @@ class ArtifactRequirement:

url: str | None = None
jar: JvmArtifactJarSourceField | None = None
excludes: frozenset[str] | None = None
chrisjrn marked this conversation as resolved.
Show resolved Hide resolved

@classmethod
def from_jvm_artifact_target(cls, target: Target) -> ArtifactRequirement:
Expand All @@ -157,6 +160,7 @@ def from_jvm_artifact_target(cls, target: Target) -> ArtifactRequirement:
"`ArtifactRequirement.from_jvm_artifact_target()` only works on targets with "
"`JvmArtifactFieldSet` fields present."
)

return ArtifactRequirement(
coordinate=Coordinate(
group=target[JvmArtifactGroupField].value,
Expand All @@ -169,6 +173,17 @@ def from_jvm_artifact_target(cls, target: Target) -> ArtifactRequirement:
if target[JvmArtifactJarSourceField].value
else None
),
excludes=frozenset(target[JvmArtifactExcludeDependenciesField].value or []) or None,
)

def exclude(self, *excludes: str) -> ArtifactRequirement:
chrisjrn marked this conversation as resolved.
Show resolved Hide resolved
"""Creates a copy of this `ArtifactRequirement` with `excludes` provided.

Mostly useful for testing (`Coordinate(...).as_requirement().exclude(...)`).
"""

return dataclasses.replace(
self, excludes=self.excludes.union(excludes) if self.excludes else frozenset(excludes)
)

def to_coord_arg_str(self) -> str:
Expand All @@ -177,12 +192,14 @@ def to_coord_arg_str(self) -> str:
)

def to_metadata_str(self) -> str:
return self.coordinate.to_coord_arg_str(
{
"url": self.url or "not_provided",
"jar": self.jar.address.spec if self.jar else "not_provided",
}
)
attrs = {
"url": self.url or "not_provided",
"jar": self.jar.address.spec if self.jar else "not_provided",
}
if self.excludes:
attrs["excludes"] = ",".join(self.excludes)

return self.coordinate.to_coord_arg_str(attrs)


# TODO: Consider whether to carry classpath scope in some fashion via ArtifactRequirements.
Expand All @@ -197,7 +214,7 @@ def from_coordinates(cls, coordinates: Iterable[Coordinate]) -> ArtifactRequirem
@dataclass(frozen=True)
class GatherJvmCoordinatesRequest:
"""A request to turn strings of coordinates (`group:artifact:version`) and/or addresses to
`jar_artifact` targets into `ArtifactRequirements`."""
`jvm_artifact` targets into `ArtifactRequirements`."""

artifact_inputs: FrozenOrderedSet[str]
option_name: str
42 changes: 39 additions & 3 deletions src/python/pants/jvm/resolve/coursier_fetch.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import dataclasses
import importlib.resources
import itertools
import json
import logging
import os
Expand All @@ -22,15 +23,18 @@
from pants.engine.collection import Collection
from pants.engine.fs import (
AddPrefix,
CreateDigest,
Digest,
DigestContents,
DigestSubset,
FileContent,
FileDigest,
MergeDigests,
PathGlobs,
RemovePrefix,
Snapshot,
)
from pants.engine.internals.native_engine import EMPTY_DIGEST
from pants.engine.process import ProcessResult
from pants.engine.rules import Get, MultiGet, collect_rules, rule
from pants.engine.target import CoarsenedTargets, Target, Targets
Expand Down Expand Up @@ -289,8 +293,17 @@ def classpath_dest_filename(coord: str, src_filename: str) -> str:
@dataclass(frozen=True)
class CoursierResolveInfo:
coord_arg_strings: FrozenSet[str]
extra_args: tuple[str, ...]
digest: Digest

@property
def argv(self) -> Iterable[str]:
"""Return coursier arguments that can be used to compute or fetch this resolve.

Must be used in concert with `digest`.
"""
return itertools.chain(self.coord_arg_strings, self.extra_args)


@rule
async def prepare_coursier_resolve_info(
Expand All @@ -300,6 +313,9 @@ async def prepare_coursier_resolve_info(
# URLs, and put the files in the place specified by the URLs.
no_jars: List[ArtifactRequirement] = []
jars: List[Tuple[ArtifactRequirement, JvmArtifactJarSourceField]] = []
extra_args: List[str] = []

LOCAL_EXCLUDE_FILE = "PANTS_RESOLVE_EXCLUDES"

for req in artifact_requirements:
jar = req.jar
Expand All @@ -308,6 +324,23 @@ async def prepare_coursier_resolve_info(
else:
jars.append((req, jar))

excludes = [
(req.coordinate, exclude)
for req in artifact_requirements
for exclude in (req.excludes or [])
]

excludes_digest = EMPTY_DIGEST
if excludes:
excludes_file_content = FileContent(
LOCAL_EXCLUDE_FILE,
"\n".join(
f"{coord.group}:{coord.artifact}--{exclude}" for (coord, exclude) in excludes
).encode("utf-8"),
chrisjrn marked this conversation as resolved.
Show resolved Hide resolved
)
excludes_digest = await Get(Digest, CreateDigest([excludes_file_content]))
extra_args += ["--local-exclude-file", LOCAL_EXCLUDE_FILE]

jar_files = await Get(SourceFiles, SourceFilesRequest(i[1] for i in jars))
jar_file_paths = jar_files.snapshot.files

Expand All @@ -320,9 +353,12 @@ async def prepare_coursier_resolve_info(

to_resolve = chain(no_jars, resolvable_jar_requirements)

digest = await Get(Digest, MergeDigests([jar_files.snapshot.digest, excludes_digest]))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

excludes_digest is usually empty, I'm not sure if it's worth making this merge conditional for performance, but the code is cleaner if we always run, even in no-op cases.


return CoursierResolveInfo(
coord_arg_strings=frozenset(req.to_coord_arg_str() for req in to_resolve),
digest=jar_files.snapshot.digest,
digest=digest,
extra_args=tuple(extra_args),
)


Expand Down Expand Up @@ -368,7 +404,7 @@ async def coursier_resolve_lockfile(
CoursierFetchProcess(
args=(
coursier_report_file_name,
*coursier_resolve_info.coord_arg_strings,
*coursier_resolve_info.argv,
),
input_digest=coursier_resolve_info.digest,
output_directories=("classpath",),
Expand Down Expand Up @@ -523,7 +559,7 @@ async def coursier_fetch_one_coord(
args=(
coursier_report_file_name,
"--intransitive",
*coursier_resolve_info.coord_arg_strings,
*coursier_resolve_info.argv,
),
input_digest=coursier_resolve_info.digest,
output_directories=("classpath",),
Expand Down
27 changes: 27 additions & 0 deletions src/python/pants/jvm/resolve/coursier_fetch_integration_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -606,3 +606,30 @@ def test_user_repo_order_is_respected(rule_runner: RuleRunner) -> None:
ArtifactRequirements.from_coordinates([jai_core]),
],
)


@maybe_skip_jdk_test
def test_transitive_excludes(rule_runner: RuleRunner) -> None:
resolve = rule_runner.request(
CoursierResolvedLockfile,
[
ArtifactRequirements(
[
Coordinate(
group="com.fasterxml.jackson.core",
artifact="jackson-databind",
version="2.12.1",
)
.as_requirement()
.exclude("com.fasterxml.jackson.core:jackson-core")
]
),
],
)

entries = resolve.entries
jackson_databind = [i for i in entries if i.coord.artifact == "jackson-databind"][0]
jackson_cores = [i for i in entries if i.coord.artifact == "jackson-core"]

assert jackson_databind.coord.version == "2.12.1"
assert len(jackson_cores) == 0
14 changes: 14 additions & 0 deletions src/python/pants/jvm/target_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,19 @@ class JvmProvidesTypesField(StringSequenceField):
)


class JvmArtifactExcludeDependenciesField(StringSequenceField):
alias = "exclude_dependencies"
Copy link
Contributor

Choose a reason for hiding this comment

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

This equivalent field in Pants v1 was called excludes. exclude_dependencies seems wordy. Any reason to not use the same name as from v1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it wasn't immediately clear to me what we would be excluding, but if we're happy with excludes we can go with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I've made that change)

help = (
"A list of unversioned coordinates (i.e. `group:artifact`) that should be excluded "
"as dependencies when this artifact is resolved.\n\n"
"This does not prevent this artifact from being included in the resolve as a dependency "
"of other artifacts that depend on it, and is currently intended as a way to resolve "
"version conflicts in complex resolves.\n\n"
"These values are passed directly to Coursier, and if specified incorrectly will show a "
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may be possible to do some rudimentary sanity checking of this value, if defined, but given we don't actually implement the parse algorithm ourselves, there are limits here.

The Coursier error message is actually quite bad when parses fail (you get java.lang.String@0xetc representations instead of the actual string values in the exception logs). I'll report (and patch?!) that upstream, where we can probably get a more reasonable error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"parse error from Coursier."
)


class JvmArtifactResolveField(JvmResolveField):
help = (
"The resolve from `[jvm].resolves` that this artifact should be included in.\n\n"
Expand Down Expand Up @@ -204,6 +217,7 @@ class JvmArtifactTarget(Target):
JvmArtifactUrlField, # TODO: should `JvmArtifactFieldSet` have an `all_fields` field?
JvmArtifactJarSourceField,
JvmArtifactResolveField,
JvmArtifactExcludeDependenciesField,
)
help = (
"A third-party JVM artifact, as identified by its Maven-compatible coordinate.\n\n"
Expand Down