New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cross JDK forking #4139

Merged
merged 8 commits into from May 30, 2018

Conversation

Projects
None yet
5 participants
@eed3si9n
Member

eed3si9n commented May 3, 2018

This is a refactoring/continuation of #4136 by @2m reflecting review comments by @dwijnand

sbt:helloworld> java++ 10
[info] Reapplying settings...
sbt:helloworld> run
[info] Running (fork) Hello
[info] 10.0.1

sbt:helloworld> java++ 8
[info] Reapplying settings...

sbt:helloworld> run
[info] Running (fork) Hello
[info] 1.8.0_171

original description

This PR proposes a discovery for Java home directories implemented by scanning well known system directories for available java installations. Currently this PR only addresses Linux and MacOS. // @cunei

This PR is a first step towards cross building across different Java versions.

Discovering Java home directories is also useful when a project needs to locate rt.jar of a different java installation (for example when working around -release 8 limitaions in JDK9)

@@ -17,3 +17,9 @@ enum PluginTrigger {
AllRequirements
NoTrigger
}
type JavaVersion {

This comment has been minimized.

@eed3si9n

eed3si9n May 3, 2018

Member

Added pseudo case class for JavaVersion, instead of using String.

@@ -268,6 +268,10 @@ object Keys {
val outputStrategy = settingKey[Option[sbt.OutputStrategy]]("Selects how to log output when running a main class.").withRank(DSetting)
val connectInput = settingKey[Boolean]("If true, connects standard input when running a main class forked.").withRank(CSetting)
val javaHome = settingKey[Option[File]]("Selects the Java installation used for compiling and forking. If None, uses the Java installation running the build.").withRank(ASetting)
val discoveredJavaHomes = settingKey[Map[JavaVersion, File]]("Discovered Java home directories")
val javaHomes = settingKey[Map[JavaVersion, File]]("The user-defined additional Java home directories")

This comment has been minimized.

@eed3si9n

eed3si9n May 3, 2018

Member

Custom key is named javaHomes.

@@ -268,6 +268,10 @@ object Keys {
val outputStrategy = settingKey[Option[sbt.OutputStrategy]]("Selects how to log output when running a main class.").withRank(DSetting)
val connectInput = settingKey[Boolean]("If true, connects standard input when running a main class forked.").withRank(CSetting)
val javaHome = settingKey[Option[File]]("Selects the Java installation used for compiling and forking. If None, uses the Java installation running the build.").withRank(ASetting)
val discoveredJavaHomes = settingKey[Map[JavaVersion, File]]("Discovered Java home directories")
val javaHomes = settingKey[Map[JavaVersion, File]]("The user-defined additional Java home directories")
val fullJavaHomes = taskKey[Map[JavaVersion, File]]("Combines discoveredJavaHomes and custom javaHomes.").withRank(CTask)

This comment has been minimized.

@eed3si9n

eed3si9n May 3, 2018

Member

fullJavaHomes is a task if someone wants to side effect. (Credit to Dale)

This comment has been minimized.

@eed3si9n

eed3si9n May 3, 2018

Member

I had to switch this back to a setting since carrying the state around aggregation is pain in the ass when implementing a command.

@eed3si9n eed3si9n requested a review from dwijnand May 3, 2018

@dwijnand

This comment has been minimized.

Member

dwijnand commented May 3, 2018

I've a yak to shave about JavaVersion.. 😕

Short-term solution: can we make it an sbt.internal type, so we can iterate on it?

Long-term solution: one idea that I was thinking was to experiment modelling "java version" using a sum type to give a selection like jenv, where when I added Oracle JDK 1.8.0.144 I got the following selection of identifiers to choose from:

  • 1.8
  • 1.8.0.144
  • oracle64-1.8.0.144

So the usual pitfalls need to be avoids:

  • can't make JavaVersion a sealed trait -> means we can't add new types
  • can't give JavaVersion or its subtypes unapply

So perhaps a starting point would be to make JavaVersion a public package sbt type, add different constructors on JavaVersion (maybe apply takes "1.8", update takes "1.8.0.144", full takes "oracle64-1.8.0.144") that just return opaque JavaVersion. But that's the creation side, I'm unsure about at the usage side. 🤔

@dwijnand

This comment has been minimized.

Member

dwijnand commented May 3, 2018

I'm overthinking it. I'm designing for use cases I don't quite have clear.

Which makes me realise: what are the motivating use cases we're looking to solve with this change? Can we explore that space before committing to a stable public API?

/cc @2m who I assume can provide the Akka use cases

@eed3si9n eed3si9n changed the title from Re: Discovery of java home directories to Cross JDK forking May 3, 2018

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented May 3, 2018

Implemented java++ 10

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented May 3, 2018

@dwijnand We can iterate by changing the implementation of JavaVersion.apply(String).

@dwijnand

This comment has been minimized.

Member

dwijnand commented May 3, 2018

Can we remove its other companion object applies and methods from the public API too?

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented May 3, 2018

I think what you're talking about can be achieved by making bunch of duplicate entries in discoveredJavaHomes like e6fa634 or jEnv without being too paranoid about JavaVersion.

@dwijnand

This comment has been minimized.

Member

dwijnand commented May 3, 2018

Can we discuss the use cases before committing to new public API?

@dwijnand

This comment has been minimized.

Member

dwijnand commented May 3, 2018

Can we discuss the use cases before committing to new public API?

Or, let's just make JavaVersion sbt.internal and cross that bridge another day.

@2m

This comment has been minimized.

Member

2m commented May 3, 2018

We have a couple of use-cases in Akka:

  1. We would use the discovery of JavaHomes to locate the rt.jar of an older version. Currently the -release flag is broken in JDK9 where it does not allow acceess to Unsafe when compiling with JDK9 and with -release 8 flag. The workaround is to compile with "-source", "8", "-target", "8", "-bootclasspath", "/usr/lib/jvm/java-8-openjdk/jre/lib/rt.jar" javacFlags for which we need JavaHome discovery.

  2. Cross testing with JDK8. Since we build with JDK9, we would like to test the compiled classes running Java 8 to be sure that we did not do something that is JDK8 incompatible.

@2m 2m referenced this pull request May 3, 2018

Closed

Discovery of java home directories #4136

1 of 1 task complete
@eed3si9n

This comment has been minimized.

Member

eed3si9n commented May 3, 2018

Given the uses cases, the minor version and vendor handling of JDK variety should be considered out of scope of this PR.

Also given that JavaVersion("10") -> file(...) needs to go into build.sbt, we do need to keep bincompat, and I see no reason to put that into internal.

> java++ 10!
> run
> check 10

This comment has been minimized.

@eed3si9n

eed3si9n May 4, 2018

Member

Cross JDK testing using jabba installed openjdk 10.

@eed3si9n eed3si9n changed the base branch from 1.1.x to 1.x May 4, 2018

@dwijnand

This comment has been minimized.

Member

dwijnand commented May 4, 2018

Also given that JavaVersion("10") -> file(...) needs to go into build.sbt, we do need to keep bincompat, and I see no reason to put that into internal.

We don't need to keep bincompat if we declare the whole feature experimental, which is what I'm proposing.

@eed3si9n eed3si9n requested a review from cunei May 4, 2018

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented May 4, 2018

I was thinking how we would document this feature, and realized that we shouldn't expose JavaVersion to javaHomes key because of the compatibility with sbt 1.0, assuming we want to support javaHomes in user-defined global settings.

  1. Keys should use String, but
  2. on the other hand, JavaVersion can now be internal
@2m

2m approved these changes May 9, 2018

LGTM. Looking good. The cross building functionality is getting more and more ripe for factoring out to a more general behaviour.

2m and others added some commits Apr 18, 2018

implement cross JDK forking
```
sbt:helloworld> java++ 10
[info] Reapplying settings...
sbt:helloworld> run
[info] Running (fork) Hello
[info] 10.0.1

sbt:helloworld> java++ 8
[info] Reapplying settings...

sbt:helloworld> run
[info] Running (fork) Hello
[info] 1.8.0_171
```
jabba 0.9.6 (no sudo)
Ref shyiko/jabba#190
Bumping to jabba 0.9.6 fixes sporaditc permission issues.

@eed3si9n eed3si9n merged commit 2848770 into sbt:1.x May 30, 2018

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eed3si9n eed3si9n deleted the eed3si9n:wip/discover-java-home branch May 30, 2018

object JavaDiscoverConfig {
val linux = new JavaDiscoverConf {
val base: File = file("/usr") / "lib" / "jvm"
val JavaHomeDir = """java-([0-9]+)-.*""".r

This comment has been minimized.

@chbatey

chbatey Jul 3, 2018

FYI this regex won't match on a default Fedora Open JDK install. See akka/akka#25298

@xuwei-k

This comment has been minimized.

Member

xuwei-k commented on main/src/main/scala/sbt/internal/CrossJava.scala in 2da1aa6 Jul 8, 2018

screen shot 2018-07-08 at 22 10 35

versions is Seq[String]. v is JavaVersion

This comment has been minimized.

Member

eed3si9n replied Jul 8, 2018

O wow. contains is the worst. Thanks!

eed3si9n added a commit to eed3si9n/sbt that referenced this pull request Jul 14, 2018

@eed3si9n eed3si9n referenced this pull request Jul 14, 2018

Merged

Fixes contains bug #4264

@eed3si9n eed3si9n added the area/jdk_x label Aug 21, 2018

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