I-build 84 fails due to usage of newer than Java 17 construct in bundle with Java 17 BREE

[akurtakov]

As can be seen at https://ci.eclipse.org/releng/job/Builds/job/I-build-4.35/84/console org.eclipse.core.jobs (first issue of unknown number) uses construct that is not allowed in Java 17 (but compiled fine with ECJ till now due to eclipse-jdt/eclipse.jdt.core#3478 ).

[akurtakov]

I could handle it by moving such problematic bundles to Java 21 so no code changes are needed as the SDK already requires Java 21.
If someone disagrees and want to keep bundles at Java 17 - they are supposed to act ASAP as until this is resolved there will be no I-builds.

[mickaelistria]

I think it makes sense to use Java 21, the fact that ECJ has supported this construct ahead of time and that some bundles have started happily using it is a showcase that the construct is desired for current (and even past) development and worth bumping some BREEs.

[vogella]

+1 for moving the bundle to Java 21

[jukzi]

+1 move to 21

09:12:42  [ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:4.0.11-SNAPSHOT:compile (default-compile) on project org.eclipse.core.jobs: Compilation failure: Compilation failure: 
09:12:42  [ERROR] /home/jenkins/agent/workspace/Builds/I-build-4.35/cje-production/gitCache/eclipse.platform.releng.aggregator/eclipse.platform/runtime/bundles/org.eclipse.core.jobs/src/org/eclipse/core/internal/jobs/JobManager.java:[725] 
09:12:42  [ERROR] 	if (job.getThread() instanceof Thread t) {
09:12:42  [ERROR] 	    ^^^^^^^^^^^^^^^
09:12:42  [ERROR] Expression type cannot be a subtype of the Pattern type
09:12:42  [ERROR] 1 problem (1 error)

[jukzi]

instanceof-pattern is used that often by now, that i would even suggest to move all bundles to 21

[laeubi]

Jobs is quite central and moving it to Java 21 will make almost all other bundles instantly depend on java 21, so I would be more comfortable in fixing the wrong pattern here instead of moving it to Java 21 for no good reason (e.g. I don't see any benefit for this switch expression used here compared to a "classic" one).

So -1 from my side due to the ripple effect in platform!

[akurtakov]

@laeubi In this case you would have to fix the known failure, start I-build to uncover whether there are more and repeat till there is successful I-build as you seem to be the only one interested in doing it right now.

[laeubi]

@laeubi In this case you would have to fix the known failure, start I-build to uncover whether there are more and repeat till there is successful I-build as you seem to be the only one interested in doing it right now.

See this, IMO completely useless "improvement" just to save a simple null check:

• Don't use instance of for simple null checks eclipse.platform#1674

[laeubi]

A simple aggregator build should uncover the problem equally well:

• A simple change for testing the build #2740

[mickaelistria]

Jobs is quite central and moving it to Java 21 will make almost all other bundles instantly depend on java 21,

This argument seems invalid: other bundles can still use Java 17 as BREE if they're happy with it and could be deployed with an older version of jobs that is still Java-17 able if it's important to them.

no good reason (e.g. I don't see any benefit for this switch expression used here compared to a "classic" one).

The fact that some other people found it valuable when coding is per se a good reason. The fact that you don't use or like that style mustn't be used to diminish the rationale of other people.

However, as you started to change the code to keep compatibility with Java 17, that's probably fine merging your changes instead of bumping BREE. I would personally find it a waste of my time if I were to do it; but as long as it's someone else's time, I won't complain...

[srikanth-sankaran]

Thanks everyone for having jumped in with options as well changes to resolve this.

Process question: Is there a better way to have handled such a fix that has a potential for build breakage ? All I did in the present case was (a) satisfied myself that we are not close to a milestone build or late stage of release cycle (b) requested to trigger an early I-build.

Is there a better protocol for a co-ordinated move across components that could have been followed or this is it?

[mickaelistria]

While we can invent new workflows and protocols here, but this case seems very unfrequent and the current protocol (trigger an I-Build just after a compiler change) already has a short and efficient feedback loop IMO.

[akurtakov]

I would say this is pretty special case which I don't remember another instance of in all these years which makes it quite hard to catch in any process.
What would be best is if people happening to uncover/causing such cases is to create similar issues for I-build build failures here for tracking of the extra work needed rather than this happening by someone from "releng"

[laeubi]

I have already suggested a while back to make the batch-compiler an own deployable (and releasable!) artifact ...

[vogella]

instanceof-pattern is used that often by now, that i would even suggest to move all bundles to 21

As far as I understand the PMC decision, it is fine to move a bundle to Java 21 in case a committer sees an advantage in using the new Java features in this bundle. @akurtakov please correct me if I'm wrong.

[akurtakov]

I've suggested eclipse-pde/eclipse.pde#1136 as a way to prove such work but as I never found time for it myself nothing happens :( and we see that improvements happen only when someone actively works on them to drive them to success.

[laeubi]

This argument seems invalid: other bundles can still use Java 17 as BREE if they're happy with it and could be deployed with an older version of jobs that is still Java-17 able if it's important to them.

Yes this "argument" is always given, and mostly by people that think they are not affected, and in theory it would work. In practice actually no one ever will test such combination and they can break at any time due to different reasons.

And this issue is actually the perfect example for this, one might argue to "simply" use the older ECJ to resolve the issue here, or "simply" use a newer ECJ in the IDE to see the issue there, but both are not really easy given the current process and strong coupling of components, and bad maintained version ranges (if used at all) and singleton bundles all around.

Anyways as expsected this PR show the problem so we don't need a full I-Build to get some confidence we have possibly cached any of those.:

• https://ci.eclipse.org/platform/job/eclipse.platform.releng.aggregator/job/PR-2740/1/console

[laeubi]

instanceof-pattern is used that often by now, that i would even suggest to move all bundles to 21

As far as I understand the PMC decision, it is fine to move a bundle to Java 21 in case a committer sees an advantage in using the new Java features in this bundle. @akurtakov please correct me if I'm wrong.

And where is the "advantage" here? Saving one single local variable in exchange for a WTF moment?

Seeing getThread() instanceof Thread it was exactly the first thing that came into my mind:

[akurtakov]

As far as I understand the PMC decision, it is fine to move a bundle to Java 21 in case a committer sees an advantage in using the new Java features in this bundle. @akurtakov please correct me if I'm wrong.

That is the case. Still, I am giving opportunity for community members to step up and do the work to keep such compatibilty and as long as this happens without preventing the rest of the community that don't care about Java 17 anymore to continue progressing all is fine IMO.

[akurtakov]

Let's keep it open till we have I-build .

[srikanth-sankaran]

Anyways as expsected this PR show the problem so we don't need a full I-Build to get some confidence we have possibly cached any of those.:

• https://ci.eclipse.org/platform/job/eclipse.platform.releng.aggregator/job/PR-2740/1/console

Can you explain what exactly this console log illustrates ? I don't understand. Is it showing that none other than JobManager.java uses the offending construct ? That we don't have to iterate through first issue of unknown number till all is green ??

[akurtakov]

Next failure is :

12:17:59  [ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:4.0.11-SNAPSHOT:compile (default-compile) on project org.eclipse.jdt.core.manipulation: Compilation failure: Compilation failure: 
12:17:59  [ERROR] /home/jenkins/agent/workspace/atform.releng.aggregator_PR-2740/eclipse.jdt.ui/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/code/SourceAnalyzer.java:[330] 
12:17:59  [ERROR] 	if (node.getQualifier() instanceof Name qualifier) {
12:17:59  [ERROR] 	    ^^^^^^^^^^^^^^^^^^^
12:17:59  [ERROR] Expression type cannot be a subtype of the Pattern type
12:17:59  [ERROR] -> [Help 1]

I'll to the java 21 bump for it as JDT committer.

[laeubi]

Just for the record this seems to be introduced by this commit in the last release.

I really wonder how such things can slip through, do we have java 21 compliance by default somewhere?

[merks]

Indeed it looks pervasive!

[vogella]

NOTE: My (now deleted) regex is not correct, it is only in which the pattern matching when the type is already of the tested one - that was not allowed in Java 17. Not sure how to find theses cases with pattern matching.

[merks]

Yes, Alex mentioned that as well. Sorry, sorry, sorry for adding noise that is unhelpful!

[akurtakov]

Next failure is (bundle [358/513]):

8:11:51  [ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:4.0.11-SNAPSHOT:compile (default-compile) on project org.eclipse.debug.tests: Compilation failure: Compilation failure: 
18:11:51  [ERROR] /home/jenkins/agent/workspace/atform.releng.aggregator_PR-2740/eclipse.platform/debug/org.eclipse.debug.tests/src/org/eclipse/debug/tests/console/IOConsoleTests.java:[140] 
18:11:51  [ERROR] 	if (status.getException() instanceof Throwable e) {
18:11:51  [ERROR] 	    ^^^^^^^^^^^^^^^^^^^^^
18:11:51  [ERROR] Expression type cannot be a subtype of the Pattern type

As this is test bundle I am doing the change now.

[merks]

Thanks @akurtakov for your conscientious, methodical effort to get us back on track!

[akurtakov]

Next one (388/513):

19:16:24  [ERROR] /home/jenkins/agent/workspace/atform.releng.aggregator_PR-2740/eclipse.jdt.debug/org.eclipse.jdt.debug.jdi.tests/tests/org/eclipse/debug/jdi/tests/NullConsoleReader.java:[48] 
19:16:24  [ERROR] 	if (out instanceof PrintStream o) {
19:16:24  [ERROR] 	    ^^^
19:16:24  [ERROR] Expression type cannot be a subtype of the Pattern type
19:16:24  [ERROR] 1 problem (1 error)

[HannesWell]

As said, we have used that pattern very often. And it is also used in open PRs. They may even reintroduce it.

You are not alone: https://www.youtube.com/watch?v=zg8xM0xxFa8&t=1820s

[akurtakov]

https://ci.eclipse.org/platform/job/eclipse.platform.releng.aggregator/job/PR-2740/7/console succeeded.
Let's see whether https://ci.eclipse.org/releng/job/Builds/job/I-build-4.35/85/ will be successful.

[akurtakov]

Closing. If there are any further issues they are past the build phase so to be handled separately https://download.eclipse.org/eclipse/downloads/drops4/I20250110-1400/

[srikanth-sankaran]

Many thanks @akurtakov for driving this to completion. Much obliged.

[srikanth-sankaran]

Tests finished on two platforms all green and this is all platform independent code. Looking good!

(https://download.eclipse.org/eclipse/downloads/drops4/I20250110-1800/)

[HannesWell]

Process question: Is there a better way to have handled such a fix that has a potential for build breakage ? All I did in the present case was (a) satisfied myself that we are not close to a milestone build or late stage of release cycle (b) requested to trigger an early I-build.

In my opinion I think in general it would help if you would run the releng.aggregator build in PRs that change ECJ with the changes of ECJ applied. Similar to like the build for this repo is doing it. This way youl would catch 'breaking' changes even before a PR is submitted. The same would apply for changes that change the produced byte-code and therefore require enforced qualifier bumps.
It probably wouldn't totally avoid them (sometimes changes are necessary), but I think it would make you aware of it earlier.