-
-
Notifications
You must be signed in to change notification settings - Fork 636
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] Refactor consumption of JVM tool lockfiles #14239
[internal] Refactor consumption of JVM tool lockfiles #14239
Conversation
The type is only being used to set up tools—whether those be external like Scalafmt or internal like our JVM parser. This rename makes that clear, which will allow us to start adding tool-specific functionality. # 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]
This is how we've been using it de facto, including in two rules where there are 2 tools used in the same process. This allows us to simplify callsites / remove verbosity # 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]
This is necessary to not break caching. # 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]
# 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]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catching out the fact that we never have multiple lockfiles in a classpath request was good. This definitely simplifies the consuming code.
@@ -680,16 +681,13 @@ class MaterializedClasspathRequest: | |||
""" | |||
|
|||
prefix: str | None = None | |||
lockfiles: tuple[CoursierResolvedLockfile, ...] = () | |||
artifact_requirements: tuple[ArtifactRequirements, ...] = () | |||
lockfile: CoursierResolvedLockfile = CoursierResolvedLockfile(entries=()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is SO much clearer. Nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely!
While there have been a lot of good cleanups, it would be good to connect further cleanup to the next feature we're trying to implement here. AFAICT, #13374 is finished, and we don't have any more lockfile stuff scheduled on the JVM side (https://github.com/pantsbuild/pants/projects/22?fullscreen=true). |
Things are still broken:
This is all in service of fixing that. Along with making things better than they were before - Chris and I collectively spent two weeks on this lockfile code because we were confused, so we're trying to clean things up so the next person in the future can approach it more easily. |
Three improvements:
MaterializedClasspath
toToolClasspath
. The type is only being used to set up tools—whether those be external like Scalafmt or internal like our JVM parser. This rename makes that clear, which will allow us to start adding tool-specific functionality.ToolClasspathRequest
is 1-per-tool. This is how we've been using it de facto, including in two rules where there are 2 tools used in the same process. This allows us to simplify callsites / remove verbosity.open()
. This is necessary to not break caching.Tool lockfiles still have issues - a followup will fix these:
scalac_plugins.py
is not using lockfile validation