code review comments

[ci skip-rust]

[ci skip-build-wheels]

src/python/pants/backend/java/subsystems/junit.py

@@ -1,14 +1,24 @@
 # Copyright 2021 Pants project contributors (see CONTRIBUTORS.md).
 # Licensed under the Apache License, Version 2.0 (see LICENSE).
-
+from pants.jvm.resolve.jvm_tool import JvmToolBase
 from pants.option.custom_types import shell_str
-from pants.option.subsystem import Subsystem
+from pants.util.docutil import git_url
 
 
-class JUnit(Subsystem):
+class JUnit(JvmToolBase):
     options_scope = "junit"
     help = "The JUnit test framework (https://junit.org)"
 
+    default_version = "5.7.2"
+    default_artifacts = [
+        "org.junit.platform:junit-platform-console:1.7.2",
+        "org.junit.jupiter:junit-jupiter-engine:%VERSION%",
+        "org.junit.vintage:junit-vintage-engine:%VERSION%",
+    ]
+    default_lockfile_resource = ("pants.backend.java.test", "junit.default.lockfile.txt")
+    default_lockfile_path = "src/python/pants/backend/java/test/junit.default.lockfile.txt"
+    default_lockfile_url = git_url(default_lockfile_path)
+
     @classmethod
     def register_options(cls, register):
         super().register_options(register)

src/python/pants/backend/java/test/junit.py

@@ -15,8 +15,7 @@
 from pants.jvm.classpath import Classpath
 from pants.jvm.jdk_rules import JdkSetup
 from pants.jvm.resolve.coursier_fetch import MaterializedClasspath, MaterializedClasspathRequest
-from pants.jvm.resolve.jvm_tool import JvmToolBase, JvmToolLockfileRequest, JvmToolLockfileSentinel
-from pants.util.docutil import git_url
+from pants.jvm.resolve.jvm_tool import JvmToolLockfileRequest, JvmToolLockfileSentinel
 from pants.util.logging import LogLevel
 
 logger = logging.getLogger(__name__)
@@ -29,23 +28,8 @@ class JavaTestFieldSet(TestFieldSet):
     sources: JavaTestSourceField
 
 
-class JunitTool(JvmToolBase):
-    options_scope = "junit-tool"
-    help = "JUnit tool configuration"
-
-    default_version = "5.7.2"
-    default_artifacts = [
-        "org.junit.platform:junit-platform-console:1.7.2",
-        "org.junit.jupiter:junit-jupiter-engine:%VERSION%",
-        "org.junit.vintage:junit-vintage-engine:%VERSION%",
-    ]
-    default_lockfile_resource = ("pants.backend.java.test", "junit.default.lockfile.txt")
-    default_lockfile_path = "src/python/pants/backend/java/test/junit.default.lockfile.txt"
-    default_lockfile_url = git_url(default_lockfile_path)
-
-
 class JunitToolLockfileSentinel(JvmToolLockfileSentinel):
-    options_scope = JunitTool.options_scope
+    options_scope = JUnit.options_scope
 
 
 @rule(desc="Run JUnit", level=LogLevel.DEBUG)
@@ -55,14 +39,13 @@ async def run_junit_test(
     junit: JUnit,
     test_subsystem: TestSubsystem,
     field_set: JavaTestFieldSet,
-    junit_tool: JunitTool,
 ) -> TestResult:
     classpath = await Get(Classpath, Addresses([field_set.address]))
     junit_classpath = await Get(
         MaterializedClasspath,
         MaterializedClasspathRequest(
             prefix="__thirdpartycp",
-            lockfiles=(junit_tool.resolved_lockfile(),),
+            lockfiles=(junit.resolved_lockfile(),),
         ),
     )
     merged_digest = await Get(
@@ -118,10 +101,8 @@ async def setup_junit_debug_request(_field_set: JavaTestFieldSet) -> TestDebugRe
 
 
 @rule
-async def generate_junit_lockfile_request(
-    _: JunitTool, junit_tool: JunitTool
-) -> JvmToolLockfileRequest:
-    return JvmToolLockfileRequest.from_tool(junit_tool)
+async def generate_junit_lockfile_request(junit: JUnit) -> JvmToolLockfileRequest:
+    return JvmToolLockfileRequest.from_tool(junit)
 
 
 def rules():

src/python/pants/jvm/resolve/jvm_tool.py

@@ -40,9 +40,6 @@ class JvmToolBase(Subsystem):
     # artifact version if it has not been specified for a particular requirement. (Subclasses must set.)
     default_artifacts: ClassVar[Sequence[str]]
 
-    # Default extra requirements for the tool. (Subclasses do not need to override.)
-    default_extra_artifacts: ClassVar[Sequence[str]] = []
-
     # Default resource for the tool's lockfile. (Subclasses must set.)
     default_lockfile_resource: ClassVar[tuple[str, str]]
 
@@ -72,41 +69,22 @@ def register_options(cls, register):
                 "The string %VERSION% version will be substituted with the --version value."
             ),
         )
-        register(
-            "--extra-artifacts",
-            type=list,
-            member_type=str,
-            advanced=True,
-            default=cls.default_extra_artifacts,
-            help="Any additional artifact requirement strings to use with the tool. This is useful if the "
-            "tool allows you to install plugins or if you need to constrain a dependency to "
-            "a certain version.",
-        )
         register(
             "--lockfile",
             type=str,
             default=cls.default_lockfile_path,
             advanced=True,
-            # TODO: Fix up the help text to not be Python-focused.
             help=(
                 "Path to a lockfile used for installing the tool.\n\n"
                 f"Set to the string `{DEFAULT_TOOL_LOCKFILE}` to use a lockfile provided by "
-                "Pants, so long as you have not changed the `--version` and "
-                "`--extra-requirements` options, and the tool's interpreter constraints are "
-                "compatible with the default. Pants will error or warn if the lockfile is not "
-                "compatible (controlled by `[python].invalid_lockfile_behavior`). See "
-                f"{cls.default_lockfile_url} for the default lockfile contents.\n\n"
+                "Pants, so long as you have not changed the `--version` option. "
+                f"See {cls.default_lockfile_url} for the default lockfile contents.\n\n"
                 f"Set to the string `{NO_TOOL_LOCKFILE}` to opt out of using a lockfile. We "
                 f"do not recommend this, though, as lockfiles are essential for reproducible "
                 f"builds.\n\n"
                 "To use a custom lockfile, set this option to a file path relative to the "
-                f"build root, then run `./pants generate-lockfiles "
+                f"build root, then run `./pants jvm-generate-lockfiles "
                 f"--resolve={cls.options_scope}`.\n\n"
-                "Lockfile generation currently does not wire up the `[python-repos]` options. "
-                "If lockfile generation fails, you can manually generate a lockfile, such as "
-                "by using pip-compile or `pip freeze`. Set this option to the path to your "
-                "manually generated lockfile. When manually maintaining lockfiles, set "
-                "`[python].invalid_lockfile_behavior = 'ignore'`."
             ),
         )
 
@@ -121,10 +99,6 @@ def artifacts(self) -> tuple[Coordinate, ...]:
             for s in self.options.artifacts
         )
 
-    @property
-    def extra_artifacts(self) -> tuple[Coordinate, ...]:
-        return tuple(Coordinate.from_coord_str(s) for s in self.options.extra_artifacts)
-
     @property
     def lockfile(self) -> str:
         f"""The path to a lockfile or special string '{DEFAULT_TOOL_LOCKFILE}'."""
@@ -161,7 +135,7 @@ class JvmToolLockfileRequest:
     @classmethod
     def from_tool(cls, tool: JvmToolBase) -> JvmToolLockfileRequest:
         return cls(
-            artifacts=FrozenOrderedSet((*tool.artifacts, *tool.extra_artifacts)),
+            artifacts=FrozenOrderedSet(tool.artifacts),
             resolve_name=tool.options_scope,
             lockfile_dest=tool.lockfile,
         )