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] Inline ValidatedJvmToolLockfileRequest into ToolClasspathRequest #14241

Merged
merged 5 commits into from
Jan 25, 2022

Conversation

Eric-Arellano
Copy link
Contributor

To consume a tool lockfile, we before would:

  1. Read the lockfile, either by:
    1. Using ValidatedJvmToolLockfileRequest, which only works if it's a JvmToolBase
    2. Using GenerateJvmLockfile -> CoursierResolvedLockfile rule, which we used with scalac_plugins.py
  2. Pass the lockfile to ToolClasspathRequest -> ToolClasspath

The rule in 1.2 was not using the validation added in #14185, and it was generally confusing to have two code paths.

This unifies everything by having ToolClasspathRequest -> ToolClasspath now handle reading, validating, and then fetching the lockfile.

Reading and validating a lockfile requires the same metadata we need for generating a lockfile with generate-lockfiles via GenerateJvmLockfile. So, we reuse that type for the sake of DRY.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Contributor

@chrisjrn chrisjrn left a comment

Choose a reason for hiding this comment

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

Seems fine :)

GenerateJvmLockfileFromTool,
ValidatedJvmToolLockfileRequest,
)
from pants.jvm.resolve.common import ArtifactRequirements, Coordinate, GatherJvmCoordinatesRequest
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 👍 👍 👍 👍 👍

@Eric-Arellano Eric-Arellano enabled auto-merge (squash) January 24, 2022 23:08
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
[ci skip-rust]
[ci skip-build-wheels]
@Eric-Arellano Eric-Arellano merged commit 6056232 into pantsbuild:main Jan 25, 2022
@Eric-Arellano Eric-Arellano deleted the jvm-tool-consumption-pt2 branch January 25, 2022 22:14
@wisechengyi wisechengyi mentioned this pull request Jan 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants