Do not automatically detect JVMs on CI systems #577
Conversation
|
@akurtakov @iloveeclipse @mickaelistria can you review/merge? |
...pse.jdt.launching/launching/org/eclipse/jdt/internal/launching/DetectVMInstallationsJob.java
Outdated
Show resolved
Hide resolved
Could those be fixed instead?
While this might be true for our Platform test suite, I'm skeptical this is generally true enough to get part oof the actual logic deployed to all users. |
I can't think about any case where I want an automatic JVM detection happen on a CI server. Configuring this on each and every test is not really suitable for a platform component. |
Imagine someone doing JDT plugins who just enjoy that the new JVM is discovered and used by the test cases they wrote just by adding it to their environment (eg Dockerfile, Jenkinsfile). This story seems quite interesting to me: no need to configure tests to get new encvironments available to use them; the tests also behave more like the "regular" IDE end-users have so the gap between the real scenario and the reproduce is much slimmer. |
In this imaginary case one can set On the other hand all the real use-cases that are unluckily include |
This is a very probable case. Before taking a decision on the default behavior, one need to consider impact on the main consumers of JDT: what makes you think that none of Spring Tools, Lombok extensions, JDT-LS... would interpret this as a regression?
One piece of advice: never claim you know all the real use-cases ;)
The issue is that this will be hard to discover and understand for consumers who need that.
Can this be improved ? Is there a real reason why this VM detection operation would be so expensive? IMO, all that effort is about putting effort on a potentially harmful or annoying workaround instead of fixing issues. Not the best way forward for anyone. |
Please give concrete hints then where they explicitly rely on automatic detection of JVMs in their CI build... For me the regression is/was already added with the feature what already has wasted numerous hours of analysis and multi spreads around to disable the feature on CI. And in particular why setting a systemproperty then is not possible at all. Then compare to all the "minor" consumers of Eclipse Platform that unluckily have JDT included in their product / tests.
Currently e.g. platform test regularly fail with OOM, @akurtakov has fixed this for one particular case where we do not need JDT but thats not always the case. Even @iloveeclipse has said JDT itself has to disable this for all their tests explicitly as it causes constant trouble, so how could one assert it is generally useful (or needed) for others if even JDT itself disable it..?!?
Yes that is why I have created this PR ;-) There is literally no benefit on a CI for this (even though claimed otherwise), as the default JVM is already detected without this feature and for the other 0.0001% of cases where
There is an easy workaround to specify the JVM property.
The harmful thing is that test are using more time and more ram depending on where and how many JVMs are installed on a CI server even if none if them are referenced anywhere in the build configuration. |
|
Again, the only thing you're talking about here are Eclipse Platform tests on Eclipse infra CI; this is a narrow use-case compared to all the cases where JDT is used. If this is an issue for Platform tests (and only for it as far as we know at the moment), then the fix should be placed somewhere around Platform tests, not in the production code. |
And you are claiming use-cases where none exits, this only disables detection of JVMs on CI servers, and I can't find any one asking for that anywhere nor that it was the intend on the initially or that is useful in any way inside a CI build! |
| if (!CI_ENV_VARIABLE.isEmpty() && Boolean.parseBoolean(System.getenv(CI_ENV_VARIABLE))) { | ||
| return Status.OK_STATUS; | ||
| } |
There was a problem hiding this comment.
Enablement logic should be placed with other similar code, in initialize.
There was a problem hiding this comment.
valid request + org.eclipse.jdt.internal.launching.DetectVMInstallationsJob.initialize() already has a flag "DetectVMInstallationsJob.disabled" which can be used for this purpose. On top of that there is a setting org.eclipse.jdt.internal.launching.LaunchingPlugin.PREF_DETECT_VMS_AT_STARTUP, i don't see a value in introducing even a third option
There was a problem hiding this comment.
The main point is that this is something everyone needs to know and then needs to set ... so one probably wastes a lot of time trying to find out why Tycho build is "unstable"... then probably one is lucky enough to map it to this feature and set the option hopefully not forget it next time again.
But yes initialize is probably a better place.
One can never say something doesn't exist unless they can prove it. I'm just agnostically cautious where you try to quickly and dogmatically extrapolate one single and narrow impression as a general rule. |
|
As said for the very uncommon case where one really want to test that specific feature you can simply set the system property and you are done. On the other hand every other project (e.g. m2e, pde, and hundred others) are affected by this just because they run their test on an eclipse-platform. An even worse there builds behave differently when something is installed on the system. |
|
So you're moving from "not any" to "imaginary" to "uncommon". As I see progress in this discussion, let's continue ;) This feature is part of something bigger which is the story of how the IDE integrates with the environment. In the tests, we do control the environment to match the ones of users and get the tests on a user-level (most of them are integration tests). We don't want to test the detection of VM downstream (that's JDT's duty to test the feature it ships), but to rely on it for our testing, just like we want users to rely on it. |
|
As you said it is the duty of JDT to test the feature itself, so I assume it is working and there is no need to "test" it all over again and again on completely unrelated parts, you still have not show where this feature is actually used in any test.
The only workaround is that one constantly need to disable this feature as it creates problem on CI system in numerous ways, just because it seem useful for a small group (e.g. jdt-ls) also does not imply it is useful in general, e.g. there is actually no request for such feature over years (apart for IDE use-cases). |
Exactly, we don't test it, we just use it in test.
People who don't want this feature can disable, that fine. People who don't have problems with it should be able to benefit from it just like end-users do; CI or not.
Let's do basic logic: the initial "We don't want it on CI Platform => No one wants it on CI Platform" statement that drove you to build this patch is wrong. I demonstrated it wrong with 1 counter example and anyway, even without counter-example, this statement could not be proven or made reliable. Now you seem to assume that "it's not useful in general => it's useful to noone", then this is pure boolean wrong. If you re-start the brainstorming from those assertions and their correct answer, you'll understand the harm of such change better. Let's imagine an analogy: tomorrow, the Temurin people decide that for their own CI, they want to skip garbage collection because it slows them down, or confuse some log reading or whatever fair reason to consider it. One of its developers decide -like you suggest here- to put in the code of this final delivery that's shipped to million users such a condition |
|
I nowhere state that this is for "Platform CI", that was your assumption, I just said that even platform gets trouble and disable the feature everywhere.
Your analogy is just the reverse, JDT has enabled a feature that no one has expected to affect their CI builds, now "million of users" are affected just because it is claimed that jdt-ls (what is not even an "official" jdt project) might require it (still no reference to any test that actually using/requiring/asserting that feature...)
It does not because Github action install it to |
|
I have not read through all the discussion. Can i summarize as followed? Votes to change default for CI builds: 2 (Hannes, Laeubi) Sounds like CI would better opt in than opt out and i don't know a single project that would opt in |
@mickaelistria do you plan to work on that? |
laeubi
force-pushed
the
dont_detect_jvms_on_ci
branch
3 times, most recently
from
5b0bd37 to
1bf78d9
Compare
|
I have now adjusted the PR and slightly modified the commit message:
If one do not want autodetection on CI, simply do nothing (off by default). If one absolutely wants this (regardless how unlikley, uncommon, whatever), could set |
laeubi
force-pushed
the
dont_detect_jvms_on_ci
branch
2 times, most recently
from
76ef9f6 to
f428af0
Compare
If one creates a proper issue report, I would probably. But it looks like most people prefer to just disable it before investigating or even reporting... |
|
https://github.com/search?q=user%3Aeclipse-platform+user%3Aeclipse-jdt+DetectVMInstallationsJob&type=issues |
...pse.jdt.launching/launching/org/eclipse/jdt/internal/launching/DetectVMInstallationsJob.java
Show resolved
Hide resolved
jukzi
requested review from
iloveeclipse
and removed request for
mickaelistria and
HannesWell
I have especially these one in mind:
Where here the automatic JVM detection was explicitly suspected to be (one possible) culprit: that's also the reason I really like to get this out of the equation (but leave it to other to opt-in). |
Automatic detection on CI systems can cause a lot of trouble (e.g. memory consumption, time to read the jvms, old jvms lingering around) and is generally not useful as a CI has usually a dedicated and fixed setup to run with. This adds a check for the common 'CI' environment variable that is usually defined by all usual CI/CD pipelines to indicate a job is running on a CI server to skip the detection regardless of preferences configuration. If one absolutely want the feature even for CI, the system-property 'DetectVMInstallationsJob.disabled' can be set to false to restore previous behavior.
laeubi
force-pushed
the
dont_detect_jvms_on_ci
branch
from
f428af0 to
f59c7a5
Compare
Then best is to leave an approving review to indicate this. |
|
If you look at the issue that went the deepest into technical analysis, it's probably #283 , and it would look like the OOM was happening again only on Eclipse infra (with its dozens of VMs https://wiki.eclipse.org/Jenkins#JDK ). To me, the problem that one needs to fix -if related- is specific to the Eclipse CI infra (and some projects already configure the disablement of auto-detection as a workaround). |
Will you do it @mickaelistria? I do understand both sides of frustration: |
Automatic detection on CI systems can cause a lot of trouble (e.g. memory consumption, time to read the jvms, old jvms lingering around) and is generally not useful as a CI has usually a dedicated and fixed setup to run with.
This adds a check for the common 'CI' environment variable that is usually defined by all usual CI/CD pipelines to indicate a job is running on a CI server to skip the detection regardless of preferences configuration. If one absolutely want the feature even for CI, the system-property 'DetectVMInstallationsJob.disabled' can be set to false to restore previous behavior.
For reference: