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

Jump to bottom

[internal] Refactor jvm_artifact usage #13890

Merged
merged 2 commits into from
Dec 16, 2021
Merged

[internal] Refactor jvm_artifact usage #13890

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
ea90f06
[internal] Refactor `jvm_artifact` usage
Eric-Arellano Dec 15, 2021
3be484e
Use Target Validation
Eric-Arellano Dec 15, 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
86 changes: 27 additions & 59 deletions src/python/pants/jvm/goals/coursier.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
from dataclasses import dataclass
from typing import Sequence

from pants.base.glob_match_error_behavior import GlobMatchErrorBehavior
from pants.engine.addresses import Address
from pants.engine.console import Console
from pants.engine.fs import (
Expand Down Expand Up @@ -47,7 +46,9 @@ def register_options(cls, register):
"--names",
type=list,
help=(
"A list of resolve names to resolve. If not provided, resolve all known resolves."
"A list of resolve names to resolve.\n\n"
"Each name must be defined as a resolve in `[jvm].resolves`.\n\n"
"If not provided, resolve all known resolves."
),
)

Expand Down Expand Up @@ -123,55 +124,23 @@ async def coursier_generate_lockfile(
)

resolved_lockfile = await Get(
CoursierResolvedLockfile,
ArtifactRequirements,
artifact_requirements,
CoursierResolvedLockfile, ArtifactRequirements, artifact_requirements
)
resolved_lockfile_json = resolved_lockfile.to_json()

lockfile_path = jvm.options.resolves[request.resolve]

# Materialise the existing lockfile, and check for changes. We don't want to re-write
# identical lockfiles
Comment on lines -134 to -135
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were duplicating the two if statement bodies. I inverted the conditional to make this all shorter and simpler.

existing_lockfile_source = PathGlobs(
[lockfile_path],
glob_match_error_behavior=GlobMatchErrorBehavior.ignore,
)
existing_lockfile_digest_contents = await Get(
DigestContents, PathGlobs, existing_lockfile_source
)

if not existing_lockfile_digest_contents:
# The user defined the target and resolved it, but hasn't created a lockfile yet.
# For convenience, create the initial lockfile for them in the specified path.
return CoursierGenerateLockfileResult(
digest=await Get(
Digest,
CreateDigest(
(
FileContent(
path=lockfile_path,
content=resolved_lockfile_json,
),
)
),
)
)

existing_lockfile_json = existing_lockfile_digest_contents[0].content

if resolved_lockfile_json != existing_lockfile_json:
# The generated lockfile differs from the existing one, so return the digest of the generated one.
return CoursierGenerateLockfileResult(
digest=await Get(
Digest,
CreateDigest((FileContent(path=lockfile_path, content=resolved_lockfile_json),)),
)
)
# The generated lockfile didn't change, so return an empty digest.
return CoursierGenerateLockfileResult(
digest=EMPTY_DIGEST,
lockfile_path = jvm.resolves[request.resolve]

# If the lockfile hasn't changed, don't overwrite it.
existing_lockfile_digest_contents = await Get(DigestContents, PathGlobs([lockfile_path]))
if (
existing_lockfile_digest_contents
and resolved_lockfile_json == existing_lockfile_digest_contents[0].content
):
return CoursierGenerateLockfileResult(EMPTY_DIGEST)

new_lockfile = await Get(
Digest, CreateDigest((FileContent(lockfile_path, resolved_lockfile_json),))
)
return CoursierGenerateLockfileResult(new_lockfile)


@goal_rule
Expand All @@ -181,34 +150,33 @@ async def coursier_resolve_lockfiles(
jvm: JvmSubsystem,
workspace: Workspace,
) -> CoursierResolve:

resolves = resolve_subsystem.options.names
available_resolves = set(jvm.options.resolves.keys())
available_resolves = set(jvm.resolves.keys())
if not resolves:
# Default behaviour is to reconcile every known resolve (this is expensive, but *shrug*)
# Default behaviour is to resolve everything.
resolves = available_resolves
else:
invalid_resolve_names = set(resolves) - available_resolves
if invalid_resolve_names:
raise CoursierError(
"The following resolve names are not names of actual resolves: "
f"{invalid_resolve_names}. The valid resolve names are {available_resolves}."
"The following resolve names are not defined in `[jvm].resolves`: "
f"{invalid_resolve_names}\n\n"
f"The valid resolve names are: {available_resolves}"
)

results = await MultiGet(
Get(CoursierGenerateLockfileResult, CoursierGenerateLockfileRequest(resolve=resolve))
Get(CoursierGenerateLockfileResult, CoursierGenerateLockfileRequest(resolve))
for resolve in resolves
)

# For performance reasons, avoid writing out files to the workspace that haven't changed.
results_to_write = tuple(result for result in results if result.digest != EMPTY_DIGEST)
if results_to_write:
merged_digest = await Get(
Digest, MergeDigests(result.digest for result in results_to_write)
merged_snapshot = await Get(
Snapshot, MergeDigests(result.digest for result in results_to_write)
)
workspace.write_digest(merged_digest)
merged_digest_snapshot = await Get(Snapshot, Digest, merged_digest)
for path in merged_digest_snapshot.files:
workspace.write_digest(merged_snapshot.digest)
for path in merged_snapshot.files:
console.print_stderr(f"Updated lockfile at: {path}")

return CoursierResolve(exit_code=0)
Expand Down
50 changes: 13 additions & 37 deletions src/python/pants/jvm/resolve/coursier_fetch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
)
from pants.engine.process import BashBinary, Process, ProcessResult
from pants.engine.rules import Get, MultiGet, collect_rules, rule
from pants.engine.target import InvalidTargetException, Target, Targets
from pants.engine.target import Target, Targets
from pants.engine.unions import UnionRule
from pants.jvm.compile import (
ClasspathEntry,
Expand Down Expand Up @@ -136,46 +136,22 @@ class ArtifactRequirement:
@classmethod
def from_jvm_artifact_target(cls, target: Target) -> ArtifactRequirement:
if not JvmArtifactFieldSet.is_applicable(target):
raise CoursierError(
raise AssertionError(
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 represents a programmer error.

"`ArtifactRequirement.from_jvm_artifact_target()` only works on targets with "
"`JvmArtifactFieldSet` fields present."
)

group = target[JvmArtifactGroupField].value
if not group:
raise InvalidTargetException(
f"The `group` field of {target.alias} target {target.address} must be set."
)

artifact = target[JvmArtifactArtifactField].value
if not artifact:
raise InvalidTargetException(
f"The `artifact` field of {target.alias} target {target.address} must be set."
)

version = target[JvmArtifactVersionField].value
if not version:
raise InvalidTargetException(
f"The `version` field of {target.alias} target {target.address} must be set."
)
Comment on lines -144 to -160
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Target API already was checking that it's not None. We just needed to teach MyPy by setting value: str, as MyPy doesn't understand that required = True means the value is no longer Optional.


url = target[JvmArtifactUrlField].value
jar_ = target[JvmArtifactJarSourceField]
jar = jar_ if jar_.value else None

if url and url.startswith("file:"):
raise CoursierError(
"Pants does not support `file:` URLS. Instead, use the `jar` field to specify the "
"relative path to the local jar file."
)

if url and jar:
raise CoursierError(
"You cannot specify both a `url` and `jar` for the same `jvm_artifact`."
)
Comment on lines -172 to -175
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now done with the Target API via Target.validate(). The major benefit of that is the validation is eager, ./pants list :: will check it. We generally have preferred eager errors.

The downside is that this is less reslient to creating a new JvmArtifact-like target in a plugin, they might not have implemented Target.validate too.


return ArtifactRequirement(
coordinate=Coordinate(group=group, artifact=artifact, version=version), url=url, jar=jar
coordinate=Coordinate(
group=target[JvmArtifactGroupField].value,
artifact=target[JvmArtifactArtifactField].value,
version=target[JvmArtifactVersionField].value,
),
url=target[JvmArtifactUrlField].value,
jar=(
target[JvmArtifactJarSourceField]
if target[JvmArtifactJarSourceField].value
else None
),
)

def to_coord_str(self, versioned: bool = True) -> str:
Expand Down
82 changes: 53 additions & 29 deletions src/python/pants/jvm/target_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,49 @@
from __future__ import annotations

from abc import ABCMeta
from typing import Optional

from pants.engine.addresses import Address
from pants.engine.target import (
COMMON_TARGET_FIELDS,
FieldSet,
InvalidFieldException,
InvalidTargetException,
SingleSourceField,
StringField,
StringSequenceField,
Target,
)
from pants.util.docutil import git_url

# -----------------------------------------------------------------------------------------------
# Generic resolve support fields
# -----------------------------------------------------------------------------------------------
Comment on lines +22 to +24
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 section is moved, unmodified, from below. I'll need it in a followup PR defined here so that I can subclass it lower in the file.



class JvmCompatibleResolvesField(StringSequenceField):
alias = "compatible_resolves"
required = False
help = (
"The set of resolve names that this target is compatible with.\n\n"
"Each name must be defined as a resolve in `[jvm].resolves`.\n\n"
"Any targets which depend on one another must have at least one compatible resolve in "
"common. Which resolves are actually used in a build is calculated based on a target's "
"dependees."
)


class JvmResolveField(StringField):
alias = "resolve"
required = False
help = (
"The name of the resolve to use when building this target.\n\n"
"Each name must be defined as a resolve in `[jvm].resolves`.\n\n"
"If not supplied, the default resolve will be used. Otherwise, one resolve that is "
"compatible with all dependency targets will be used."
)


# -----------------------------------------------------------------------------------------------
# `jvm_artifact` targets
# -----------------------------------------------------------------------------------------------
Expand All @@ -27,6 +59,7 @@
class JvmArtifactGroupField(StringField):
alias = "group"
required = True
value: str
help = (
"The 'group' part of a Maven-compatible coordinate to a third-party JAR artifact.\n\n"
"For the JAR coordinate `com.google.guava:guava:30.1.1-jre`, the group is "
Expand All @@ -37,6 +70,7 @@ class JvmArtifactGroupField(StringField):
class JvmArtifactArtifactField(StringField):
alias = "artifact"
required = True
value: str
help = (
"The 'artifact' part of a Maven-compatible coordinate to a third-party JAR artifact.\n\n"
"For the JAR coordinate `com.google.guava:guava:30.1.1-jre`, the artifact is `guava`."
Expand All @@ -46,6 +80,7 @@ class JvmArtifactArtifactField(StringField):
class JvmArtifactVersionField(StringField):
alias = "version"
required = True
value: str
help = (
"The 'version' part of a Maven-compatible coordinate to a third-party JAR artifact.\n\n"
"For the JAR coordinate `com.google.guava:guava:30.1.1-jre`, the version is `30.1.1-jre`."
Expand Down Expand Up @@ -76,6 +111,17 @@ class JvmArtifactJarSourceField(SingleSourceField):
"Use the `url` field for remote artifacts."
)

@classmethod
def compute_value(cls, raw_value: Optional[str], address: Address) -> Optional[str]:
value_or_default = super().compute_value(raw_value, address)
if value_or_default and value_or_default.startswith("file:"):
raise InvalidFieldException(
f"The `{cls.alias}` field does not support `file:` URLS, but the target "
f"{address} sets the field to `{value_or_default}`.\n\n"
"Instead, use the `jar` field to specify the relative path to the local jar file."
)
return value_or_default


class JvmArtifactPackagesField(StringSequenceField):
alias = "packages"
Expand Down Expand Up @@ -109,7 +155,6 @@ class JvmProvidesTypesField(StringSequenceField):


class JvmArtifactFieldSet(FieldSet):

group: JvmArtifactGroupField
artifact: JvmArtifactArtifactField
version: JvmArtifactVersionField
Expand Down Expand Up @@ -137,6 +182,13 @@ class JvmArtifactTarget(Target):
"That is, an artifact identified by its `group`, `artifact`, and `version` components."
)

def validate(self) -> None:
if self[JvmArtifactJarSourceField].value and self[JvmArtifactUrlField].value:
raise InvalidTargetException(
f"You cannot specify both the `url` and `jar` fields, but both were set on the "
f"`{self.alias}` target {self.address}."
)


# -----------------------------------------------------------------------------------------------
# JUnit test support field(s)
Expand All @@ -145,31 +197,3 @@ class JvmArtifactTarget(Target):

class JunitTestSourceField(SingleSourceField, metaclass=ABCMeta):
"""A marker that indicates that a source field represents a JUnit test."""


# -----------------------------------------------------------------------------------------------
# Generic resolve support fields
# -----------------------------------------------------------------------------------------------


class JvmCompatibleResolvesField(StringSequenceField):
alias = "compatible_resolves"
required = False
help = (
"The set of resolve names that this target is compatible with.\n\n"
"Each name must be defined as a resolve in `[jvm].resolves`.\n\n"
"Any targets which depend on one another must have at least one compatible resolve in "
"common. Which resolves are actually used in a build is calculated based on a target's "
"dependees."
)


class JvmResolveField(StringField):
alias = "resolve"
required = False
help = (
"The name of the resolve to use when building this target.\n\n"
"Each name must be defined as a resolve in `[jvm].resolves`.\n\n"
"If not supplied, the default resolve will be used. Otherwise, one resolve that is "
"compatible with all dependency targets will be used."
)