Skip to content

Conversation

@wiwa
Copy link
Contributor

@wiwa wiwa commented Apr 6, 2022

Description

Like with #1373, we enable runtime_jdk selection on scala_binary targets.
This specifies the java binary to use with bazel run on a per-target level.

Motivation

Similar to #1373, we want to enable gradual migration of scala_binary targets to be compatible with any JDK upgrades.

@wiwa wiwa requested review from liucijus and simuons as code owners April 6, 2022 19:26
Co-authored-by: Shane Delmore <[email protected]>
@wiwa wiwa force-pushed the f/bin-runtime-jdk branch from 5a661c1 to e2c6f2e Compare April 6, 2022 19:27

# Make sure scala_binary respects runtime_jdk on bazel run
scala_binary(
name = "scala_binary_jdk_11",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Win! Could you see if we can add bazel run <this target> to somewhere like this as a test? https://github.com/bazelbuild/rules_scala/blob/master/test_rules_scala.sh

possibly test/shell/test_scala_binary.sh?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This target is added to the list of sh_tests above, so it will be run as a test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh ok. missed that. thank you!

Copy link
Collaborator

@liucijus liucijus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @wiwa!

@wiwa
Copy link
Contributor Author

wiwa commented Apr 7, 2022

@simuons is this mergeable?

Copy link
Collaborator

@simuons simuons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @wiwa and sorry for delay.

@simuons simuons merged commit d789f0a into bazel-contrib:master Apr 8, 2022
@ShaneDelmore
Copy link
Contributor

No problem at all, thanks for merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants