Skip to content
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

Support discovering JDK 11 in JAVA_HOME for CrossJava #4313

Merged
merged 3 commits into from Sep 17, 2018

Conversation

@raboof
Copy link
Contributor

commented Aug 9, 2018

  • Allow an optional '-' after the 'jdk' prefix in the directory name
  • Allow discovering JDK's in $JAVA_HOME
  • Allow discovering JDK's in C://Program Files/Java
@@ -367,11 +369,42 @@ private[sbt] object CrossJava {
}
}

class WindowsDiscoverConfig extends JavaDiscoverConf {
val base: File = file("C://Program Files/Java")

This comment has been minimized.

Copy link
@kxbmap

kxbmap Aug 10, 2018

You can use sys.env.get("ProgramFiles").

In 64-bit machine, %ProgramFiles% points to:

  • C:\Program Files if running sbt with 64-bit Java
  • C:\Program Files (x86) if running sbt with 32-bit Java

In 32-bit machine, %ProgramFiles% points to C:\Program Files.

This comment has been minimized.

Copy link
@raboof

raboof Aug 13, 2018

Author Contributor

That variable isn't filled for me (sbt 1.2.1 on cygwin on windows8)?

Should we just look in C:\Program Files (x86)/Java as well as C:\Program Files/Java?

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Aug 14, 2018

Member

Looking for both makes sense.

This comment has been minimized.

Copy link
@raboof

raboof Aug 15, 2018

Author Contributor

Updated the PR to look in both.

To follow up on my last comment: the environment variable is %PROGRAMFILES% (case-sensitive). This always points to C:\Program Files on 32-bits machines. On 64-bits machines it points to C:\Program Files or C:\Program Files (x86) depending on your current process (https://stackoverflow.com/questions/17688758/using-programfilesx86-on-windows-os-32bit).

Can we invoke 32-bit Java from 64-bit sbt and vice-versa?

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Aug 15, 2018

Member

Can we invoke 32-bit Java from 64-bit sbt and vice-versa?

I am not sure if we tested specifically for these scenarios, but I'm guessing it's not too different from calling JDK 11 from JDK 8: You invoke java.exe in another process, and hope that serialization still hold up.

This comment has been minimized.

Copy link
@raboof

raboof Aug 15, 2018

Author Contributor

Right: then checking for both architectures seems to be the reasonable thing to do (even if it might then pick the 'wrong' one)

Copy link
Member

left a comment

Could you address @kxbmap's comment on Windows please?

@raboof

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2018

Yes, hold on while I set up a windows vm to be able to test :)

@eed3si9n

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

[error] C:\projects\sbt\main\src\test\scala\sbt\internal\CrossJavaTest.scala:55:22: not enough arguments for constructor WindowsDiscoverConfig: (base: java.io.File)sbt.internal.CrossJava.JavaDiscoverConfig.WindowsDiscoverConfig.
[error] Unspecified value parameter base.
[error]       val conf = new WindowsDiscoverConfig {
[error]                      ^
[warn] two warnings found
[error] one error found
[error] (mainProj / Test / compileIncremental) Compilation failed
@raboof

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2018

oops

@eed3si9n eed3si9n added the review label Aug 21, 2018
@eed3si9n eed3si9n changed the base branch from 1.x to develop Aug 31, 2018
@eed3si9n eed3si9n merged commit 04a4456 into sbt:develop Sep 17, 2018
3 checks passed
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
@raboof raboof deleted the raboof:supportJdk11AndJavaHomeInCrossJava branch Oct 28, 2018
@eed3si9n eed3si9n added this to the 1.3.0 milestone Mar 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.