[internal] add `JvmToolBase` for lockfile handling for JVM tools #13777

tdyas · 2021-12-02T03:32:26Z

Add JvmToolBase as base class for subsystems that handle lockfiles for JVM tools. Adds jvm-generate-lockfiles goal for rebuilding JVM tool lockfiles.

Port the junit test goal to use JvmToolBase.

tdyas · 2021-12-02T19:19:54Z

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

+    """Base class for subsystems that configure a set of artifact requirements for a JVM tool."""
+
+    # Default version of the tool. (Subclasses must set.)
+    default_version: ClassVar[str]


The intent here is to provide a simpler way for users to configure the tool version. The version is then substituted into default_artifacts via a %VERSION% placeholder.

There has been a growing convention around using Python's default templating for things: so just {version} see abb022b.

tdyas · 2021-12-02T19:20:47Z

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

+
+    default_version = "5.7.2"
+    default_artifacts = [
+        "org.junit.platform:junit-platform-console:1.7.2",


Unclear to me how best to handle multiple different versions in the artifacts vis-a-vis the --version option.

Do you mean collisions between the tool and the user's classpath? Eventually we'll likely need to implement shading to handle that: #13259.

No, just the artifacts having different versions. Note that the "main" version number is 5.7.2 but the junit-platform-console artifact has version 1.7.2. Maybe --version arguments should be defined by the individual tools based on their needs?

Oh, I see what you are saying. Hm. It may be better not to include the version yet then...? It's an easy feature to add later, and it is pretty error prone here (although I do think that this case is a bit exceptional).

tdyas · 2021-12-02T19:30:48Z

Open question: Should this handle user resolve lockfiles? I wanted to reduce the scope of this change so didn't try to merge coursier-resolve yet into the jvm-generate-lockfiles goal.

chrisjrn

This seems sound, but I'll wait for tests etc before approving

chrisjrn · 2021-12-02T19:22:18Z

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

+from pants.util.logging import LogLevel
+from pants.util.ordered_set import FrozenOrderedSet
+
+DEFAULT_TOOL_LOCKFILE = "<default>"


Now that we're doing this in two places, we should probably extract a config constants file and have python_tool_base draw from it as well.

I think this should probably go in core/goals/generate_lockfiles.py once we unify the langauges

chrisjrn · 2021-12-02T19:28:46Z

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

+
+class GenerateJvmLockfilesSubsystem(GoalSubsystem):
+    name = "jvm-generate-lockfiles"
+    help = "Generate lockfiles for JVM tools third-party dependencies."


Per #13374, we want generate-lockfiles to work on all supported platforms, so this may not just be for third-party dependencies.

(if it's possible to remove coursier-resolve during this work, I'd support that!)

See the open question I just posted. #13777 (comment)

This PR is an intermediate step toward merging everything. The question is not should we merge everything, but rather how many PRs to get there.

(moving inline)

Open question: Should this handle user resolve lockfiles? I wanted to reduce the scope of this change so didn't try to merge coursier-resolve yet into the jvm-generate-lockfiles goal.

No, it shouldn't merge with user lockfiles (i.e. generate-user-lockfile). But this change should merge with the Python tool lockfile goal: i.e. generate-lockfiles. That would align it with the status quo.

As discussed offline, fine with doing some of this merging as a fast follow.

Want me to handle merging the two goals? I'm now working on lockfiles as my main project so the context switch isn't bad for me

tdyas · 2021-12-02T19:36:08Z

This seems sound, but I'll wait for tests etc before approving

Added test coverage to the same level as the Python goal. There doesn't seem to be much in the way of actual tests of resolving the Python tool lockfiles support. I.e., searching PythonToolLockfileSentinel in tests only shows up in src/python/pants/backend/python/goals/lockfile_test.py.

tdyas · 2021-12-02T19:38:30Z

The actual testing of tool lockfiles seems to be from the fact that the tools that have been ported to them actually still continue to pass their own tests.

chrisjrn · 2021-12-02T19:41:33Z

I'm happy to punt on #13374, but it's probably something we want to address sooner than later, now that we have coursier-resolve, jvm-generate-lockfiles and generate-lockfiles. This is probably not a pattern we want to continue.

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

stuhood · 2021-12-02T20:15:28Z

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

+    """Base class for subsystems that configure a set of artifact requirements for a JVM tool."""
+
+    # Default version of the tool. (Subclasses must set.)
+    default_version: ClassVar[str]


There has been a growing convention around using Python's default templating for things: so just {version} see abb022b.

stuhood · 2021-12-02T20:19:07Z

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

+    # Default artifacts for the tool in GROUP:NAME format. The `--version` value will be used for the
+    # artifact version if it has not been specified for a particular requirement. (Subclasses must set.)
+    default_artifacts: ClassVar[Sequence[str]]


I'm a bit concerned that we'll need to deprecate or complicate this syntax if we add more features to jvm_artifact (excludes, etc)... it would be good to already know what the migration path from this syntax to specifying these arguments with a target will be.

It seems like it should be possible to support both simultaneously, but let's just start by assuming that we will need to add target support, and that maybe this syntax stays around as the "easy mode" way to specify deps?

Should we have a syntax for differentiating between the use cases? Or would we first try to interpret as an address and then fallback to interpreting as a coordinate?

Maybe we should just go right to addresses only to start?

Based on discussion yesterday, I think that we'll always need a way to specify the builtin version, and even if we did have a "synthetic target" based approach, we'd still need options to seed the target. So this seems fine for now.

Maybe we should just go right to addresses only to start?

Maybe, yea? In the absence of synthetic targets though, that would amount to having two codepaths: one for builtin requirements, the other for targets. But probably not too much complexity, honestly.

Hmm, would we be fine with deprecating the coordinate syntax later on?

Yes, although obviously easier to do before stabilizing the JVM. Sorry to wobble back and forth, but I do sortof think that accepting addresses here would be straightforward... I think that you could just request classpath entries for them with ClasspathEntryRequest.for_targets, and it would automatically support either compiling a tool or downloading it.

No worries. Will implement address support then, with fallback to coordinate syntax.

Using ClasspathEntryRequest.for_targets though doesn't seem to generate coordinates needed to invoke Coursier though to obtain a lockfile. Instead it obtains actual jars to form the invokable classpath. Am I missing something?

My thought to support addresses is to scan the transitive dependencies of the user-supplied addresses and extract jvm_artifacts and extract their coordinates. Not sure how this would play with url= option on those targets though.

Oh, doh. Yes, would only be able to consume jvm_artifacts initially. Sorry about that.

stuhood · 2021-12-02T20:22:10Z

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

+        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.",
+        )


IMO, let's not add this until we know we need it... only some Python tools do, and the option ends up with a different meaning in each case. It's likely that we'd want to add specific options to subclasses with their own help, which could then override an empty method of the superclass to append them, for example.

will remove

stuhood · 2021-12-02T20:22:33Z

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

+            type=str,
+            default=cls.default_lockfile_path,
+            advanced=True,
+            # TODO: Fix up the help text to not be Python-focused.


stuhood · 2021-12-02T20:25:49Z

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

+
+class GenerateJvmLockfilesSubsystem(GoalSubsystem):
+    name = "jvm-generate-lockfiles"
+    help = "Generate lockfiles for JVM tools third-party dependencies."


(moving inline)

Open question: Should this handle user resolve lockfiles? I wanted to reduce the scope of this change so didn't try to merge coursier-resolve yet into the jvm-generate-lockfiles goal.

No, it shouldn't merge with user lockfiles (i.e. generate-user-lockfile). But this change should merge with the Python tool lockfile goal: i.e. generate-lockfiles. That would align it with the status quo.

stuhood · 2021-12-03T17:54:27Z

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

+
+    default_version = "5.7.2"
+    default_artifacts = [
+        "org.junit.platform:junit-platform-console:1.7.2",


Oh, I see what you are saying. Hm. It may be better not to include the version yet then...? It's an easy feature to add later, and it is pretty error prone here (although I do think that this case is a bit exceptional).

stuhood · 2021-12-03T17:56:18Z

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

+    # Default artifacts for the tool in GROUP:NAME format. The `--version` value will be used for the
+    # artifact version if it has not been specified for a particular requirement. (Subclasses must set.)
+    default_artifacts: ClassVar[Sequence[str]]


Based on discussion yesterday, I think that we'll always need a way to specify the builtin version, and even if we did have a "synthetic target" based approach, we'd still need options to seed the target. So this seems fine for now.

stuhood · 2021-12-03T17:57:13Z

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

+    # assert_filtered(disabled_tool, resolve_specified=False)
+    # with pytest.raises(ValueError) as exc:
+    #     assert_filtered(disabled_tool, resolve_specified=True)
+    # assert f"`[{disabled_tool.resolve_name}].lockfile` is set to `{NO_TOOL_LOCKFILE}`" in str(
+    #     exc.value
+    # )


[ci skip-rust] [ci skip-build-wheels]

stuhood

Thanks again.

[ci skip-rust] [ci skip-build-wheels]

Eric-Arellano

Sweet, thanks. Pardon the delayed review

Eric-Arellano · 2021-12-06T23:22:19Z

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

+from pants.util.logging import LogLevel
+from pants.util.ordered_set import FrozenOrderedSet
+
+DEFAULT_TOOL_LOCKFILE = "<default>"


I think this should probably go in core/goals/generate_lockfiles.py once we unify the langauges

Eric-Arellano · 2021-12-06T23:24:57Z

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

+        with open(lockfile_path, "rb") as f:
+            return f.read()


This doesn't seem safe with caching?

I didn't really consider the caching aspect. Another thing to fix ...

I'm addressing this as part of #13827.

Eric-Arellano · 2021-12-06T23:26:13Z

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

+
+class GenerateJvmLockfilesSubsystem(GoalSubsystem):
+    name = "jvm-generate-lockfiles"
+    help = "Generate lockfiles for JVM tools third-party dependencies."


Want me to handle merging the two goals? I'm now working on lockfiles as my main project so the context switch isn't bad for me

Internal changes: * [internal] Remove superfluous f-string specifiers ([#13821](#13821)) * [internal] scala: extract annotations as consumed types ([#13810](#13810)) * [jvm] Split nailgun digest from input file digests ([#13813](#13813)) * [internal] jvm: add jre_major_version and use stderr to properly extract version ([#13812](#13812)) * [internal] Clean up Go `embed` support's handling of dependencies ([#13801](#13801)) * [internal] scala: handle package object syntax in parser ([#13809](#13809)) * [internal] java: fix junit sentinel rule setup ([#13815](#13815)) * [internal] upgrade to rust v1.57.0 ([#13807](#13807)) * [internal] add failing test for FrozenDict equality issue ([#13389](#13389)) * [internal] Use `PyObject` instead of `Value` in more places ([#13802](#13802)) * Remove MultiPlatform Process abstractions ([#12725](#12725)) * [internal] add `JvmToolBase` for lockfile handling for JVM tools ([#13777](#13777)) * [internal] Port `MergeDigests` to Rust ([#13773](#13773)) * [jvm] Spawn nailgun servers outside the pool lock ([#13796](#13796)) * [internal] DRY loading internal Go binaries ([#13800](#13800)) * [internal] Convert unit tests to use pytest ([#13798](#13798)) * [internal] remove dead code - socket util. ([#13797](#13797)) * [internal] Reorganize Go parser scripts ([#13791](#13791)) * Adds Jackson core/datatype to `JVM_ARTIFACT_MAPPINGS` ([#13792](#13792)) * [internal] go: initial support for embedding resources ([#13743](#13743)) * [internal] Refer to `go.mod` path when downloading packages ([#13786](#13786)) * [internal] More robust Go dependency inference tests ([#13785](#13785)) * [internal] `tailor` doesn't add `go_package` for `testdata` folder ([#13783](#13783)) * [internal] Add Scala backend to dryrun for wheel builds. ([#13772](#13772)) * [internal] Unify JVM thirdparty resolves ([#13771](#13771)) * [internal] scala: infer dependencies from consumed symbols and intermediate scopes ([#13758](#13758)) * [internal] java: infer scala encoded symbols ([#13739](#13739)) * [internal] scala: parse and report package scopes ([#13738](#13738)) * [internal] go: configure included env vars for `GoSdkProcess` ([#13734](#13734)) * Fix some `./cargo audit` vulnerabilities. ([#13728](#13728)) * [internal] fix non-empty __init__.py ([#13730](#13730)) * Compute RepositoryPex directly from addresses. ([#13716](#13716)) * Upgrade to cargo-audit 0.16.0. ([#13729](#13729)) * Simplify `NativeHandler`. ([#13727](#13727)) * [internal] Switch to a maintained fork of the `fuse` crate for `brfs`. ([#13679](#13679)) * [internal] Add infrastructure to support deprecating field names ([#13666](#13666)) * [internal] Introduce OptionalPex/OptionalPexRequest. ([#13712](#13712)) * [internal] tailor adds go_package targets ([#13703](#13703)) * [internal] Remove unused testproject for pants-plugin ([#13704](#13704)) * [internal] Rename ambiguous `subpath` variable for Go code ([#13701](#13701)) * [internal] scala: generate the JVM names seen by Java code for Scala code ([#13696](#13696)) * Use RequirementsPexRequest in run_pex_binary.py. ([#13693](#13693)) * [internal] Refactor finding owning `go_mod` for first-party packages ([#13695](#13695)) * [internal] repl: add append_only_caches / run_in_workspace attributes to ReplRequest ([#13599](#13599)) * [internal] switch back to official `cargo-ensure-prefix` crate ([#13692](#13692)) * [internal] scala: extract type names from all Type.* AST nodes ([#13685](#13685)) * [internal] Convert unit tests to use pytest ([#13652](#13652)) * Unblock codegen support for java export analysis (#13645) ([#13675](#13675)) * [internal] upgrade to Rust 2021 Edition ([#13644](#13644)) * [internal] Don't store derived values on `go_first_party_package` targets ([#13676](#13676)) * Upgrade to py03 0.15.1. ([#13725](#13725)) * Add PyPDF2 to module mapping ([#13717](#13717)) * Upgrade console and indacatif. ([#13726](#13726))

tdyas force-pushed the jvm_tool_base branch 2 times, most recently from 529bfa3 to 3d40177 Compare December 2, 2021 19:14

tdyas changed the title ~~[WIP] [internal] add JvmToolBase for lockfile handling for JVM tools~~ [internal] add JvmToolBase for lockfile handling for JVM tools Dec 2, 2021

tdyas marked this pull request as ready for review December 2, 2021 19:18

tdyas requested review from stuhood, Eric-Arellano and chrisjrn December 2, 2021 19:18

tdyas commented Dec 2, 2021

View reviewed changes

chrisjrn reviewed Dec 2, 2021

View reviewed changes

chrisjrn approved these changes Dec 2, 2021

View reviewed changes

tdyas commented Dec 2, 2021

View reviewed changes

src/python/pants/backend/java/test/junit.py Outdated Show resolved Hide resolved

stuhood reviewed Dec 2, 2021

View reviewed changes

stuhood reviewed Dec 3, 2021

View reviewed changes

Tom Dyas added 4 commits December 3, 2021 16:24

JVM tool base + first user

7a0223f

[ci skip-rust] [ci skip-build-wheels]

add tests

a5a7153

[ci skip-rust] [ci skip-build-wheels]

code review comments

2cfc63f

[ci skip-rust] [ci skip-build-wheels]

accept target addresses in addition to coordinate strings

c66c196

[ci skip-rust] [ci skip-build-wheels]

tdyas force-pushed the jvm_tool_base branch from d689c16 to c66c196 Compare December 3, 2021 21:24

Tom Dyas added 2 commits December 3, 2021 18:32

help text updates

b263210

[ci skip-rust] [ci skip-build-wheels]

revert change to address.py

4aaf6a0

[ci skip-rust] [ci skip-build-wheels]

tdyas requested review from stuhood and chrisjrn December 3, 2021 23:54

stuhood approved these changes Dec 4, 2021

View reviewed changes

add test for tool lockfile resolution

c8376ae

[ci skip-rust] [ci skip-build-wheels]

tdyas merged commit d1486c4 into pantsbuild:main Dec 4, 2021

tdyas deleted the jvm_tool_base branch December 4, 2021 06:08

stuhood mentioned this pull request Dec 6, 2021

Add JVM tool base #13754

Closed

illicitonion mentioned this pull request Dec 6, 2021

Prep for 2.9.0.dev3 #13823

Merged

Eric-Arellano reviewed Dec 6, 2021

View reviewed changes

[internal] add JvmToolBase for lockfile handling for JVM tools #13777

[internal] add JvmToolBase for lockfile handling for JVM tools #13777

Conversation

tdyas commented Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdyas commented Dec 2, 2021 • edited Loading

chrisjrn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdyas Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdyas commented Dec 2, 2021

tdyas commented Dec 2, 2021

chrisjrn commented Dec 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdyas Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[internal] add `JvmToolBase` for lockfile handling for JVM tools #13777

[internal] add `JvmToolBase` for lockfile handling for JVM tools #13777

tdyas commented Dec 2, 2021 •

edited

Loading

tdyas commented Dec 2, 2021 •

edited

Loading

tdyas Dec 2, 2021 •

edited

Loading

tdyas Dec 3, 2021 •

edited

Loading