-
Notifications
You must be signed in to change notification settings - Fork 59
Add current java.home to detected installations #657
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
base: master
Are you sure you want to change the base?
Conversation
|
@basilevs by the way, what do you think about this one ? |
This works for me without the patch, as long as I launch Eclipse for Java as a built product and not from a launch configuration. Is this change really necessary? I will test again, to be sure. |
|
To me, with latest JustJ snapshot for Java 24, this patch is necessary for justj to be added. |
|
Note that other VMs were already detected before that (maybe it has an influence whether the list of installed VMs is initially empty or not...) |
|
@mickaelistria could you somehow fix the problem mentioned in #231 (comment) ? The patch is otherwise good. |
Well, as I'm not a committer, I don't think it would be faster for me to get it merged than for you. So as you identified the problem, I think it's better if you can provide a patch to get proper tracking of who actually did the hard part (ie detecting the problem). |
Neither am I. Are you not the author of the original #231 where the collection in question was introduced?
I have no solution in mind yet. I can create one later, but I've thought that as the collection is touched here, it would make sense to fix it too. |
Indeed I am, yet I am not allowed to merge quickfixes with a faster process.
Most people around Eclipse or JDT project prefer finer grain PRs/commits with a single purpose (so it makes it easier to understand the changes when looking at the history). So having the fix in another PR would be fine. |
|
@SougandhS As you seem active on this topic, would you please be able to review (and hopefully merge) this one too? |
Sure 👍 |
76d4c8b to
b2e8e52
Compare
|
@mickaelistria does this only works with JustJ vendors ? when I tried with OpenJDK it detected even without patch |
|
Was OpenJDK installed in a directory that is already detected by default? Also, does the detection discover some other JVMs? |
OpenJDK is usually installed in a standard easy to discover location. To test this, run JDT with Java installed in a non-standard directory. See my comment above |
Didnt installed with any installers, just extracted the binaries to Mac's download folder and tried from there |
No actually |
|
It could be that in case no VM at all is found, then some other bit of code in JDT does add the current VM. But here, the proposal is to always add it.
Maybe it's also a mac oddity? |
Usually MacOS registers JVM when installing it from |
Does this work with Windows and Linux ? |
|
For Linux, yes, it does work for me. |
If its a Mac only issue we can ignore it for now |
Reproduced on Microsoft Windows Server 2019 Datacenter, when launching: Autodetect does not discover |
Unfortunately, the test builds don't produce artifacts (this could be changed, but that would be a different ticket to discuss). |
Yes, many system properties are different when launching from source. I'm not sure if that difference affect this PR, but it might.
Yes, for my tests it always is. |


What it does
Auto-discover the VM currently running the IDE as "Installed JRE"
How to test
Author checklist