Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
--jvm tries to find Java executable system-wide. #11500
base: develop
Are you sure you want to change the base?
--jvm tries to find Java executable system-wide. #11500
Changes from 1 commit
ca2be99
19b0c3d
b91121f
d2e8994
b38928b
b9792ab
6b54fcc
7149cf3
d6bf5a7
1455b02
586fc73
dd357ca
25d8110
c356a0c
ec38e0b
b89db13
fd52bbf
46022f2
ea3af5f
df1e2ec
ed31ffc
9fb23d7
45b7889
f14333d
184c245
fb25d57
049b991
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that work on Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't expect it to work on windows. This is a fallback that is currently only usable for devs and not anyone else. For all the other use-cases, the java exe should be found in the installed distribution runtimes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the precedence should actually look into distribution first and only afterwards use system JVM.
The whole purpose of
ensoup
was to manage JVMs related to the engine versions, so that the engine is run with the right compatible JVM and not a random version that may be incompatible with it (I guess it used to be more important before Truffle 'got unchained').I think ideally we should first to run the JVM version required by the engine if it's found in the distribution. Then we can fallback to using the logic of searching
JAVA_HOME
andPath
.Ideally I'd also check the JVM version and log a warning if the version required by engine does not match the version that has been found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version is stored in the engine's manifest - see for example
built-distribution/enso-engine-0.0.0-dev-windows-amd64/enso-0.0.0-dev/manifest.yaml
.It contains fields:
graal-vm-version
andgraal-java-version
.You can see
lib/scala/runtime-version-manager/src/main/scala/org/enso/runtimeversionmanager/components/Manifest.scala
for the logic of loading the manifest and extracting theGraalVMVersion
from it.We have a
GraalCEReleaseProvider
which translates thisGraalVMVersion
into a strategy for downloading it from GitHub - but you probably don't need this.The function
graalDirectoryForVersion
inRuntimeVersionManager
takesGraalVMVersion
and gives you back the directory name under which this JVM version will be sitting in the$ENSO_DATA_DIRECTORY/runtime
(installed byensoup
).I guess you could factor out the
Manifest
andgraalDirectoryForVersion
utilities to a common package and you could use it to figure out the JVM version tied to the engine.Btw. the manifest also contains a
jvm-options
field. I'd be very happy if we could keep the (sometimes changing) options in a single place.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if you want to check if the JVM version is correct you need to call
java --version
and somehow parse its output and compare withgraal-java-version
.e.g. I get
I guess I want to extract the second word from the first line. At least for now that seems to work. But we can update the logic whenever the version format of expected JVM changes - unexpected format just means it's not the JVM we expect :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly better way than parsing output of
java -version
is to run a small program on the JVM:and just read the output line by line without the need to parse words.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool - that indeed sounds better, I didn't think of that