Add support for multiple compile/testing JDKs #14421

chrisjrn · 2022-02-09T23:52:35Z

This adds support for a jdk_version on all JVM source targets as well as deploy_jar. Supply --jvm-default-source-jdk to set a default for the jdk_version field -- currently we also fall back to --jvm-jdk, but this is deprecated and will be removed presently.

Needs: tests

There's a bit more work than I'd like to support validating that a deploy_jar is run with an acceptable JDK version, so deferring that for a follow-up.

…mits # 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]

…jdk-setup-indirection

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

…le-jdks-take-2 [ci skip-rust]

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

… sources [ci skip-rust] [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]

…le-jdks-take-2

# 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]

src/python/pants/backend/codegen/protobuf/scala/rules.py

src/python/pants/backend/java/target_types.py

src/python/pants/jvm/jdk_rules.py

stuhood · 2022-02-10T00:06:29Z

src/python/pants/jvm/jdk_rules.py

+    # TODO: verify that we're requesting the same JDK version for all `ct` members?
+    t = ct.representative


Yea. This validation could maybe move into ClasspathEntryRequest, and then become a field of ClasspathEntryRequest? Not a blocker though.

Good catch! I think I'd decided on CoarsenedTarget before I knew where JDK selection would be necessary. I'll see if I can refactor that appropriately.

@stuhood I think the validation side of things will require ClasspathEntryRequest.for_targets to be re-written as a rule (or further complicated by accepting more singleton arguments) to resolve the JDK at construction time. If you're OK with that, I can do that in a separate PR before landing this?

src/python/pants/jvm/subsystems.py

# 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]

…get` # 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]

…le-jdks-take-2 [ci skip-rust]

# 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]

chrisjrn · 2022-02-10T19:53:25Z

Requesting a full review now (working on tests in the meantime). Supporting validation/erroring out in the right place is going to take a bit more work than I'd like, and I'd rather land the overall support.

# 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]

chrisjrn · 2022-02-10T19:58:49Z

src/python/pants/jvm/jdk_rules.py


 @dataclass(frozen=True)
 class Nailgun:
    classpath_entry: ClasspathEntry


+@dataclass(frozen=True)
+class JdkForClasspathRequest:


This could -- theoretically -- get folded into JdkRequest, but I feel like that's going to overcomplicate things further

(commented on the topic elsewhere before seeing this)

stuhood

Thanks! One blocking issue here for the repl goal. I also think that we should swap up the option names.

stuhood · 2022-02-10T21:40:20Z

src/python/pants/jvm/jdk_rules.py

+@rule
+async def jdk_request_for_classpath(request: JdkForClasspathRequest) -> JdkRequest:
+
+    # TODO: verify that we're requesting the same JDK version for all `ct` members?
+    t = request.cper.component.representative
+
+    if not t.has_field(JvmJdkField):
+        raise ValueError(f"Cannot construct a JDK request for a non-JVM target {t}")
+
+    field = t[JvmJdkField]
+
+    return JdkRequest(field)


This rule is overly coarse I think (and possibly worth fixing before landing): the request will be different for every CoarsenedTarget, even though all the rule is going to consume is a single field.

I'd suggest moving the body of this @rule into a static method of JdkRequest that constructs a JdkRequest from a CoarsenedTarget. Alternatively, keep the JdkForClasspathRequest type and move the body to static method there. Either way, that should reduce the number of times this rule runs to "once per distinct field value".

src/python/pants/backend/scala/goals/repl.py

stuhood · 2022-02-10T21:43:25Z

src/python/pants/jvm/jdk_rules.py


 @dataclass(frozen=True)
 class Nailgun:
    classpath_entry: ClasspathEntry


+@dataclass(frozen=True)
+class JdkForClasspathRequest:


(commented on the topic elsewhere before seeing this)

src/python/pants/jvm/jdk_rules.py

src/python/pants/jvm/subsystems.py

Co-authored-by: Stu Hood <stuhood@gmail.com>

# 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]

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

chrisjrn

This addresses the two most egregious issues; I'll work on efficiency as a fast-follow for this.

stuhood

Thanks!

src/python/pants/backend/java/compile/javac_test.py

src/python/pants/jvm/subsystems.py

Co-authored-by: Stu Hood <stuhood@gmail.com>

# 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]

…le-jdks-take-2 [ci skip-rust] [ci skip-build-wheels]

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

stuhood · 2022-02-11T06:13:05Z

@chrisjrn : It looks like your test failures are likely a collision with the second commit in #14441... sorry about that. In short: "target generator" types like java_sources no longer keep their resolve field: it's moved to the targets that they generate. To account for that, that commit adjusted things to ignore targets which don't have a resolve field, rather than failing.

# Conflicts: # src/python/pants/backend/java/target_types.py # src/python/pants/backend/scala/target_types.py # 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]

chrisjrn · 2022-02-11T16:23:23Z

@stuhood Looks like the tests pass again once we treat jdk as a copied_field rather than a moved_field. AIUI, copied_field behaviour is what we previously had before the adjustment in #14441, so that's probably fine.

Let's address whether it should be made a moved_field once I start work on validation.

# 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]

…ake-2' into chrisjrn/13995-take-3 [ci skip-rust] [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]

Previously we were constructing a `JdkEnvironment` for every instance of `JvmJdkField` rather than caching by version. This refactors `JdkRequest` to no longer store a field, and instead, always store a version string or magic constant. This should reduce the number of times we download a JDK. (Fast-follow for #14421, addresses #13995)

stuhood · 2022-02-16T17:53:34Z

Let's address whether it should be made a moved_field once I start work on validation.

It should be a moved field, as that's necessary for parametrize (which is also why the changes were made in #14441). This is probably why @tdyas wasn't able to get pantsbuild/example-jvm#10 to work.

Christopher Neugebauer added 12 commits February 7, 2022 08:51

Extracts nailgun classpath request to improve caching in upcoming com…

bdab128

…mits # 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]

Adds JdkRequest object

93e3ad6

# 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]

Merge remote-tracking branch 'origin/main' into chrisjrn/13995-extra-…

c6edd29

…jdk-setup-indirection

Use version as a single parameter rather than major_version/is_system

2511a9f

Add indirection to make JdkSetup less of a lynchpin

bedf287

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

Merge remote-tracking branch 'origin/main' into chrisjrn/13995-multip…

c3ba1e8

…le-jdks-take-2 [ci skip-rust]

Make JvmProcess require a JvmEnvironment as an argument

6645d79

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

Adds JvmCompatibleJdkVersionField, and starts accepting it for Java…

c9c9a85

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

Adds JDK specifications to most JVM tools

6c12822

# 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]

Merge remote-tracking branch 'origin/main' into chrisjrn/13995-multip…

8470488

…le-jdks-take-2

Correctly handle default JDK values

00ea5e7

# 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]

Renames JdkSetup to InternalJdk/jdk_wrapper

022ed84

# 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]

chrisjrn requested a review from stuhood February 9, 2022 23:52

chrisjrn marked this pull request as draft February 9, 2022 23:57

chrisjrn changed the title ~~Chrisjrn/13995 multiple jdks take 2~~ WIP Support multiple JDKs Feb 9, 2022

stuhood reviewed Feb 10, 2022

View reviewed changes

Christopher Neugebauer added 6 commits February 10, 2022 08:24

Make InternalJdk a subclass instead of a wrapper for JdkEnvironment

962bd3f

# 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]

Simplifies the JdkRequest hierarchy

2c9410e

# 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]

Request JDK for a ClasspathEntryRequest rather than a `CoarsenedTar…

390cccc

…get` # 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]

Merge remote-tracking branch 'origin/main' into chrisjrn/13995-multip…

854e5f7

…le-jdks-take-2 [ci skip-rust]

rename the jdk field

2464332

# 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]

Amend help

940c166

# 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]

chrisjrn marked this pull request as ready for review February 10, 2022 19:53

chrisjrn requested review from tdyas and stuhood February 10, 2022 19:53

-oops

1e9656e

# 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]

chrisjrn commented Feb 10, 2022

View reviewed changes

chrisjrn changed the title ~~WIP Support multiple JDKs~~ Add support for multiple compile/testing JDKs Feb 10, 2022

stuhood requested changes Feb 10, 2022

View reviewed changes

chrisjrn and others added 4 commits February 10, 2022 15:50

Update src/python/pants/jvm/jdk_rules.py

8497e04

Co-authored-by: Stu Hood <stuhood@gmail.com>

Update src/python/pants/jvm/jdk_rules.py

bb5b605

Co-authored-by: Stu Hood <stuhood@gmail.com>

Pick the right scala JVM for the repl

d21a999

# 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]

Swaps JDK options around

185aeeb

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

chrisjrn requested a review from stuhood February 10, 2022 23:52

chrisjrn commented Feb 10, 2022

View reviewed changes

stuhood approved these changes Feb 11, 2022

View reviewed changes

src/python/pants/backend/java/compile/javac_test.py Outdated Show resolved Hide resolved

src/python/pants/jvm/subsystems.py Outdated Show resolved Hide resolved

src/python/pants/jvm/subsystems.py Outdated Show resolved Hide resolved

chrisjrn and others added 5 commits February 10, 2022 17:25

Update src/python/pants/jvm/subsystems.py

9cc181d

Co-authored-by: Stu Hood <stuhood@gmail.com>

Update src/python/pants/jvm/subsystems.py

e2ef420

Co-authored-by: Stu Hood <stuhood@gmail.com>

Fix test

5db3a86

# 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]

Merge remote-tracking branch 'origin/main' into chrisjrn/13995-multip…

81f999f

…le-jdks-take-2 [ci skip-rust] [ci skip-build-wheels]

Merge branch 'main' into chrisjrn/13995-multiple-jdks-take-2

d1c69f0

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

stuhood enabled auto-merge (squash) February 11, 2022 05:41

stuhood added a commit to stuhood/example-jvm that referenced this pull request Feb 11, 2022

Depends on pantsbuild/pants#14421

35dae81

stuhood mentioned this pull request Feb 11, 2022

Parametrize the JDK used for testing pantsbuild/example-jvm#10

Merged

stuhood added a commit to stuhood/example-jvm that referenced this pull request Feb 11, 2022

Depends on pantsbuild/pants#14421

fbd18cc

Christopher Neugebauer added 4 commits February 11, 2022 08:23

Uses JvmJdkField as a copied_field for the time being

03dfb7a

# 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]

Merge remote-tracking branch 'chrisjrn/chrisjrn/13995-multiple-jdks-t…

23a38c1

…ake-2' into chrisjrn/13995-take-3 [ci skip-rust] [ci skip-build-wheels]

jdk_rules_test was testing the wrong jdk option

c0cadb9

# 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]

🤦

d74c5d2

# 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]

stuhood merged commit aae56a6 into pantsbuild:main Feb 11, 2022

chrisjrn deleted the chrisjrn/13995-multiple-jdks-take-2 branch February 11, 2022 20:23

chrisjrn mentioned this pull request Feb 11, 2022

[internal] Improve JdkEnvironment cache performance #14452

Merged

chrisjrn mentioned this pull request Feb 14, 2022

Support for multiple JDKs #13995

Closed

stuhood added a commit to stuhood/example-jvm that referenced this pull request Mar 23, 2022

Depends on pantsbuild/pants#14421

f48619a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multiple compile/testing JDKs #14421

Add support for multiple compile/testing JDKs #14421

chrisjrn commented Feb 9, 2022 •

edited

stuhood Feb 10, 2022 •

edited

chrisjrn Feb 10, 2022

chrisjrn Feb 10, 2022

chrisjrn commented Feb 10, 2022

chrisjrn Feb 10, 2022

stuhood Feb 10, 2022

stuhood left a comment

stuhood Feb 10, 2022 •

edited

stuhood Feb 10, 2022

chrisjrn left a comment

stuhood left a comment

stuhood commented Feb 11, 2022

chrisjrn commented Feb 11, 2022

stuhood commented Feb 16, 2022

		# TODO: verify that we're requesting the same JDK version for all `ct` members?
		t = ct.representative

Add support for multiple compile/testing JDKs #14421

Add support for multiple compile/testing JDKs #14421

Conversation

chrisjrn commented Feb 9, 2022 • edited

stuhood Feb 10, 2022 • edited

Choose a reason for hiding this comment

chrisjrn Feb 10, 2022

Choose a reason for hiding this comment

chrisjrn Feb 10, 2022

Choose a reason for hiding this comment

chrisjrn commented Feb 10, 2022

chrisjrn Feb 10, 2022

Choose a reason for hiding this comment

stuhood Feb 10, 2022

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

stuhood Feb 10, 2022 • edited

Choose a reason for hiding this comment

stuhood Feb 10, 2022

Choose a reason for hiding this comment

chrisjrn left a comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

stuhood commented Feb 11, 2022

chrisjrn commented Feb 11, 2022

stuhood commented Feb 16, 2022

chrisjrn commented Feb 9, 2022 •

edited

stuhood Feb 10, 2022 •

edited

stuhood Feb 10, 2022 •

edited