Allow per-target java runtime selection for scala_junit_tests

[wiwa]

Description

Add an attr (runtime_jdk) to scala_junit_tests to specify the version of Java to run with via JavaRuntimeInfo.
Adapt java_bin() and phase_default_info to pick up the runtime and add its runfiles.

Motivation

This change allows users to test their code on different Java versions before having to adapt their code to compile with those versions.
Our specific use case is to test and run code on the JDK it will be deployed onto, supporting a smoother migration from older versions.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

[wisechengyi]

Suggested change

	def specified_java_runtime(ctx, default_runtime):
	def specified_java_runtime(ctx, default_runtime=None):

[wisechengyi]

so caller don't need to call with None

also would be good make sure the return value isn't None.

[wisechengyi]

Thanks. Looks reasonable.

Is this already 4 & 5 compatible, or is it going to break one? At the moment we probably need a compatible solution.

[wisechengyi]

so caller don't need to call with None

also would be good make sure the return value isn't None.

[wisechengyi]

Also we should probably add this new field to https://github.com/bazelbuild/rules_scala/blob/master/docs/scala_test.md when this is functionality good.

[ShaneDelmore]

Thanks. Looks reasonable.

Is this already 4 & 5 compatible, or is it going to break one? At the moment we probably need a compatible solution.

Our original version was Bazel 5 only, but we reworked it to be 4 and 5 compat.

[liucijus]

LGTM! @wiwa please fix linter errors by running ./lint.sh.

Has this improvement been tested on any other codebase than rules scala itself?

[wiwa]

Has this improvement been tested on any other codebase than rules scala itself?

Yes, it works for our use case!

[wiwa]

Also we should probably add this new field to https://github.com/bazelbuild/rules_scala/blob/master/docs/scala_test.md when this is functionality good.

We only add the runtime_jdk attr to scala_junit_test, which seems to not propagate to scala_test. We can make a followup which addresses all test rules?

[liucijus]

Also we should probably add this new field to https://github.com/bazelbuild/rules_scala/blob/master/docs/scala_test.md when this is functionality good.

We only add the runtime_jdk attr to scala_junit_test, which seems to not propagate to scala_test. We can make a followup which addresses all test rules?

Yes, it's easier to review smaller PRs

[liucijus]

Thanks, @wiwa!

[simuons]

Hi @wiwa , probably I'm too late but gonna ask anyway. Have you considered adding runtime_jdk on a toolchain and use that for all test targets. Do you really need this to be controlled per target basis?

[wisechengyi]

Hi @wiwa , probably I'm too late but gonna ask anyway. Have you considered adding runtime_jdk on a toolchain and use that for all test targets. Do you really need this to be controlled per target basis?

Hi @simuons, this is still a good question. There's a few reasons. Here is a couple of major ones:

1. Twitter has thousands of targets to migrate, some are 11 runtime compatible and some are not, so we would like to have runtime selection on target level in order to migrate the tests (PR for scala_binary to come soon) incrementally, so regression can be prevented while we move the JDK 11 migration forward.
2. Each team has their own cadence of development, testing, and deployment, so flipping the switch for everyone at the same time would likely be too risky.

[simuons]

@wiwa thanks for PR
@wisechengyi thanks for explanation.