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

Local test fails due DetectVMInstallationsJob running at startup #1190

Closed
iloveeclipse opened this issue Jun 26, 2023 · 8 comments
Closed

Local test fails due DetectVMInstallationsJob running at startup #1190

iloveeclipse opened this issue Jun 26, 2023 · 8 comments
Labels
bug Something isn't working help wanted Extra attention is needed regression Something was broken by a previous change

Comments

@iloveeclipse
Copy link
Member

          > JavaProjectTests.testAddExternalLibFolder6 still fails. see for example https://github.com/eclipse-jdt/eclipse.jdt.core/pull/1119/checks?check_run_id=14325578340 you don't need to wait for I-Build: just rebase any of my commits

I'm seeing that same failure locally, with jdt.{core,ui,debug} at current head of master. Looking at the fix here, it tells me, that only builds are fixed but not local test launches in Eclipse.

Needing to add the system property to every test launch that is potentially affected doesn't look like a good option. So, I'm re-opening this issue.

To my own surprise adding this to the static initializer of the affected test class fixes the failure for me:

System.setProperty("DetectVMInstallationsJob.disabled", "true");

But I'm not convinced, that a system property is a good way to control this in the first place. At least we'd need to find the best suitable location for this kludge.

Perhaps jdt.core needs an extension point for plugins wishing to initialize stuff after jdt.core is fully initialized, rather than activators launching initialization jobs to run at unknown point in time? Or an explicit callback for updating (compliance) options at a point in time that's suitable for jdt.core (i.e., after JavaCorePreferenceInitializer has run)?

Originally posted by @stephan-herrmann in #1150 (comment)

@iloveeclipse iloveeclipse added help wanted Extra attention is needed regression Something was broken by a previous change bug Something isn't working labels Jun 26, 2023
@mickaelistria
Copy link
Contributor

Perhaps jdt.core needs an extension point for plugins wishing to initialize stuff after jdt.core is fully initialized, rather than activators launching initialization jobs to run at unknown point in time? Or an explicit callback for updating (compliance) options at a point in time that's suitable for jdt.core (i.e., after JavaCorePreferenceInitializer has run)?

The extension point would be a bit overkill IMO.
We could delay the job "to initialize stuff after jdt.core is fully initialized", however, how to properly detect when "jdt.core is fully initialized"? Is there some reliable way to query something performing that.
Note that the job that adds VM runtimes to the registry isn't very high priority. If we can't find a proper way to delay the execution of the non-concurrent bits, then we can just schedule it with a delay of eg 2 seconds.

@iloveeclipse
Copy link
Member Author

@mickaelistria : I believe what would help in most cases if the job would check if platform UI (workbench) is started and only run in that case. Most JDT core tests affected by this job are headless, so in most cases the job will simply not need to be started at all. That would be short term solution for JDT core but not for debug/UI.

The root cause of all evil is the change of JDT core options by init of the "3rd party" jdt launching code that sets VM & auto-updates JVM / JDT core defaults. It can't be solved by a delay, because test can take a while (especially if one is debugging it).

May be that can be avoided by checking the presence of some tests related property etc, haven't checked yet what test application / runner does and if that can be detected by the job code.

@mickaelistria
Copy link
Contributor

I believe what would help in most cases if the job would check if platform UI (workbench) is started and only run in that case

The initial case that drove me there is JDT.LS which is headless but for which this feature is as much helpful as it is for Eclipse IDE. The UI is the criterion I'd like to use.

Would it help if before changing the settings, the job would query the settings or the preferences? Would it be enough to ensure that JDT core initialization is complete and changes are now possible? Or maybe we can instead only trigger the job when a workspace refresh event on a java project takes place?

@stephan-herrmann
Copy link
Contributor

Would it help if before changing the settings, the job would query the settings or the preferences?

This sounds promising to me, provided, we can identify some setting which tells clients whether the preference initializer has run to completion.

Given that AbstractPreferenceInitializer is a core runtime concept, how do other plug-ins protect against conflicting access from the initializer vs 3rd party plug-ins?

@stephan-herrmann
Copy link
Contributor

PING

@jukzi
Copy link
Contributor

jukzi commented Aug 10, 2023

The main error is that the launching Plugin sets up anything even if the launching plugin is not needed. My suggestion is to only run the DetectVMInstallationsJob lazy when any launching vm is needed. That would totally remove the ill behavior from jdt.core tests.
Only question is when is the time that that Job is needed? My guess: on org.eclipse.jdt.launching.JavaRuntime.initializeVMs(), i will propose a change.

jukzi pushed a commit to jukzi/eclipse.jdt.debug that referenced this issue Aug 10, 2023
jukzi pushed a commit to jukzi/eclipse.jdt.debug that referenced this issue Aug 14, 2023
jukzi pushed a commit to eclipse-jdt/eclipse.jdt.debug that referenced this issue Aug 14, 2023
@mickaelistria
Copy link
Contributor

@stephan-herrmann @iloveeclipse Does @jukzi 's patch fix this issue?

@stephan-herrmann
Copy link
Contributor

Does @jukzi 's patch fix this issue?

Yes, fixed. Thanks, @jukzi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed regression Something was broken by a previous change
Projects
None yet
Development

No branches or pull requests

4 participants