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

JSPs can't be compiled on Java 17+ #18

Closed
akurtakov opened this issue Jun 15, 2022 · 141 comments · Fixed by #20 or #59
Closed

JSPs can't be compiled on Java 17+ #18

akurtakov opened this issue Jun 15, 2022 · 141 comments · Fixed by #20 or #59

Comments

@akurtakov
Copy link
Member

https://bugs.eclipse.org/bugs/show_bug.cgi?id=578221 is the original bug. In order to fix our build compilation I had to do b459090 to provide latest JDT.
But this effectively means Help system fails badly without jdt.core. We should find a way to update to jasper compiler with recent enough jdt.core bundled in it.

@akurtakov
Copy link
Member Author

@tjwatson Can you give some hints how we should proceed here ? Or point to someone we can ask for help? This is general issue in Equinox Servlet support.

@tjwatson
Copy link
Contributor

What I don't get is why we need to update jdt.core used by jasper simply to run and compile on Java 17+ I would think that would only be required if the JSPs actually used Java 17 language features. Shouldn't the old jdt.core used by jasper still be able to compile existing JSPs regardless of the version of Java we are running on?

@akurtakov
Copy link
Member Author

akurtakov commented Jun 15, 2022

I didn't manage to tell Jasper what bytecode version it should compile jsp files to and it keeps using the running JVM version for that.

@iloveeclipse
Copy link
Member

Can't we precompile jsp's and ship them as classes?

@akurtakov
Copy link
Member Author

Can't we precompile jsp's and ship them as classes?

That would be workaround for our jsp files . Help system provides the capabilities for contributors to ship jsp files so these would still be broken.
P.S. Shipping precompiled jsp files is good things on it's own which I thought is working but it obviously isn't. There is even https://github.com/eclipse-platform/eclipse.platform.ua/blob/master/org.eclipse.help.webapp/pom.xml#L27 which precompiles the jsp files and they are even in the jar file but these class files are not used at runtime. This should be handled in another issue if someone wants to try it out.

@laeubi
Copy link
Contributor

laeubi commented Jun 20, 2022

From the bugzilla:

Rudolf Hornig CLA Friend 2022-03-29 13:48:29 EDT

After a bit of debugging it seems that this is caused by the fact that the org.apache.jasper.glassfish bundle contains an old copy of the CDT compiler (from 2015) that cannot use the java.lang.System class file from the JRE17 classpath and it throws an exception.

  • This does not happen if the java version is < 17.
  • This does not happen either if the org.eclipse.jdt.core bundle is installed on the system because in that case the compiler is pulled from that bundle and it contains an up-to-date compiler which can compile the JSP files just fine.

So maybe just the compiler could be provided as an additional bundle then if its too hard to update the glassfish? Maybe it would even be better if glassfish is not embedding anything at all and instead get it from the outside?

@laeubi
Copy link
Contributor

laeubi commented Jun 20, 2022

I didn't manage to tell Jasper what bytecode version it should compile jsp files to and it keeps using the running JVM version for that.

Maybe jasper should then simply use https://docs.oracle.com/javase/7/docs/api/javax/tools/JavaCompiler.html ?

@akurtakov
Copy link
Member Author

I'll probably add dependency on jdt.core as that's the only thing we can control on our side. I failed at even finding decent jasper replacement. The "original" one from Tomcat brings too many tomcat dependencies so a no-go.

@laeubi
Copy link
Contributor

laeubi commented Jun 23, 2022

Maybe its even enough to add ECJ as dependency?

@akurtakov
Copy link
Member Author

That should be org.eclipse.jdt.core.compiler.batch. If it's enough sure.

@akurtakov
Copy link
Member Author

I just checked and o.e.jdt.core.compiler.batch is 3.1MB but is not included in Eclipse SDK while o.e.jdt.core is 6.7 MB but is already there. I'm kind of torn which one to prefer.

@iloveeclipse
Copy link
Member

Would "import package" help? In SDK this would be jdt core. Other products could try to get ecj, but I'm not sure if it is even deployed in the p2 repo. I know it is provided as separated download.

@akurtakov
Copy link
Member Author

I will go that path.

@sravanlakkimsetti
Copy link
Member

I just checked and o.e.jdt.core.compiler.batch is 3.1MB but is not included in Eclipse SDK while o.e.jdt.core is 6.7 MB but is already there. I'm kind of torn which one to prefer.

ECJ is not packaged in SDK. also this consists of classes from

  1. org.eclipse.jdt.core
  2. org.eclipse.jdt.core.compiler.tool
  3. org.eclipse.jdt.core.compiler.apt

you'll probably need o.e.jdt.core.compiler.apt also for annotation processing.

@akurtakov
Copy link
Member Author

JSPs don't do annotation processing AFAICT so Import-Package: org.eclipse.jdt.core.compiler seems to do the trick at least for what I have tested.

@sravanlakkimsetti
Copy link
Member

ECJ does exist in p2 repo. but it is not packaged in any of the products. If required we can go that way. I think adding ecj to help.feature may also work.

@akurtakov
Copy link
Member Author

Please look at the PR. I would like to go as less invasive as possible here and this sounds like the minimal patch.

akurtakov added a commit that referenced this issue Jun 23, 2022
Jasper shipped embeds ancient version of ecj which doesn't support
targeting newer Java versions. As Eclipse is the author of ecj it makes
sense to include our own version which is the absolute latest one.
Removed workaround in tests explicitly adding jdt.core that is no longer
needed
Fixes #18
@laeubi
Copy link
Contributor

laeubi commented Jun 23, 2022

I'm a bit confused, does JDT not use EJC so anything that contains JDT should also include ECJ? Just wondering :-)

@akurtakov
Copy link
Member Author

I'm not an expert on the topic too but from looking at contents of jar files - ECJ seems to be called the standalone compiler at the download page while o.e.jdt.core is Eclipse plugin with dependency on core.resources and etc. Both contain org.eclipse.jdt.core.compiler package and friends but they are not 1:1 replacements.

@iloveeclipse
Copy link
Member

ECJ jar is subset of JDT core bundle, it is standalone compiler library only. So it would be enough to require ecj if it would be there, but it isn't, and shipping both bundles with SDK doesn't make sense. However, consumers not interested in JDT don't need to include it and could use ECJ only, also avoiding that they would use JDT core API that aren't running without the rest of dependencies JDT has.

@sravanlakkimsetti
Copy link
Member

Y-build failed with this error

00:47:30.594  [ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:3.0.0-SNAPSHOT:validate-classpath (default-validate-classpath) on project org.eclipse.help.webapp: Execution default-validate-classpath of goal org.eclipse.tycho:tycho-compiler-plugin:3.0.0-SNAPSHOT:validate-classpath failed: org.osgi.framework.BundleException: Bundle org.eclipse.help.webapp cannot be resolved:org.eclipse.help.webapp [36]
00:47:30.594  [ERROR]   Unresolved requirement: Import-Package: org.eclipse.jdt.core.compiler
00:47:30.594  [ERROR]     -> Export-Package: org.eclipse.jdt.core.compiler; bundle-symbolic-name="org.eclipse.jdt.core"; bundle-version="3.30.50.qualifier"; version="0.0.0"
00:47:30.594  [ERROR]        org.eclipse.jdt.core [37]
00:47:30.594  [ERROR]          Unresolved requirement: Require-Bundle: org.eclipse.core.resources; bundle-version="[3.17.0,4.0.0)"

Full log https://ci.eclipse.org/releng/job/Y-build-4.25/18/console

@iloveeclipse
Copy link
Member

jdt core requires resources, ecj not...

@akurtakov
Copy link
Member Author

@sravanlakkimsetti I'm out of ideas how to proceed as I don't want to mandate second copy of ecj. Please revert (or tell me to do so) if you want to urge Y-build.

@laeubi
Copy link
Contributor

laeubi commented Jun 23, 2022

ECJ jar is subset of JDT core bundle, it is standalone compiler library only

Why can'T we have an ECJ bundle and JDT just consume this?

@sravanlakkimsetti
Copy link
Member

@sravanlakkimsetti I'm out of ideas how to proceed as I don't want to mandate second copy of ecj. Please revert (or tell me to do so) if you want to urge Y-build.

If I-build succeeds, I don't have any problem with this one. Lets wait for the I-build and see if I-build succeeds. If not we should revert.
If I-build succeeds, then there is an issue with Y-build that I will investigate further

@iloveeclipse
Copy link
Member

Why can'T we have an ECJ bundle and JDT just consume this?

Split packages etc. This would require JDT deployment to be refactored, so JDT consumes ecj for some packages, not sure how easy that is and if that would open another can of worms.

I'm out of ideas how to proceed

Require platform resources bundle? Shouldn't be a big issue for SDK but will be if the help is used standalone. Activation of JDT bundle will trigger activation of resources, which could fail without workspace set or trigger write access to $home/workspace which might also fail...

There is a hack possible: JDT core could declare resources bundle dependency "optional", I assume no one uses JDT without resources bundle anyway. The compiler code in theory should not depend on resources (ecj runs standalone), but not sure if that also true if the entire jdt core bundle is on classpath.

iloveeclipse added a commit to iloveeclipse/eclipse.jdt.core that referenced this issue Nov 17, 2022
The org.eclipse.jdt.core.ecj.validation was a "dummy" bundle so far,
used only to validate compilation issues in IDE.

That one should be renamed (org.eclipse.jdt.core.ecj.validation ->
org.eclipse.jdt.core.compiler.batch) and be a proper maven library.

It is actually the ecj compiler library without any dependencies, that
could be consumed by JDT and the rest of the world.

It must be required and re-exported by JDT core.
Unfortunately, there are two split packages:

org.eclipse.jdt.internal.compiler
org.eclipse.jdt.internal.compiler.parser

So the new bundle exports them to jdt.core and jdt.core re-exports.

org.eclipse.jdt.compiler.apt and org.eclipse.jdt.compiler.tool were
fragments of jdt.core, now they would be inside
org.eclipse.jdt.core.compiler.batch.

TODO:

1) What I did NOT tried is to re-write all the magic scripts that build
and package separated ecj library out of jdt.core.
2) The 3 antadapter classes are now split over ecj and jdt core, because
BuildJarIndex.java and CheckDebugAttributes.java depend on JDT core
code.
3) ecj.1 and build_ecj.xml aren't touched yet. ecj.1 was touched last
time 2017, it seem to be used for man pages.
4) pom from jdt core will need an adoption.
5) org.eclipse.jdt-feature need to be updated
6) TBC

See
- eclipse-jdt#181
- eclipse-platform/eclipse.platform.ua#18
iloveeclipse added a commit to iloveeclipse/eclipse.jdt.core that referenced this issue Dec 1, 2022
The org.eclipse.jdt.core.ecj.validation was a "dummy" bundle so far,
used only to validate compilation issues in IDE.

That one is renamed (org.eclipse.jdt.core.ecj.validation ->
org.eclipse.jdt.core.compiler.batch) and is a proper maven library now.

It is actually the ecj compiler library without any dependencies (except
optional ant), that could be consumed by JDT and the rest of the world.

It must be required and re-exported by JDT core.

Unfortunately, there are two split packages:

- org.eclipse.jdt.internal.compiler
- org.eclipse.jdt.internal.compiler.parser

So the new bundle exports them to jdt.core and jdt.core re-exports.

org.eclipse.jdt.compiler.apt and org.eclipse.jdt.compiler.tool were
fragments of jdt.core, now they are inside
org.eclipse.jdt.core.compiler.batch.

TODO:

1) What I did NOT tried is to re-write all the magic scripts that build
and package separated ecj library out of jdt.core.
2) The 3 antadapter classes are now split over ecj and jdt core, because
BuildJarIndex.java and CheckDebugAttributes.java depend on JDT core
code.
3) ecj.1 and build_ecj.xml aren't touched yet. ecj.1 was touched last
time 2017, it seem to be used for man pages.
4) pom from jdt core will need an adoption.
5) org.eclipse.jdt-feature need to be updated
6) TBC

See
- eclipse-jdt#181
- eclipse-platform/eclipse.platform.ua#18
iloveeclipse added a commit to iloveeclipse/eclipse.jdt.core that referenced this issue Dec 1, 2022
The org.eclipse.jdt.core.ecj.validation was a "dummy" bundle so far,
used only to validate compilation issues in IDE.

That one is renamed (org.eclipse.jdt.core.ecj.validation ->
org.eclipse.jdt.core.compiler.batch) and is a proper maven library now.

It is actually the ecj compiler library without any dependencies (except
optional ant), that could be consumed by JDT and the rest of the world.

It must be required and re-exported by JDT core.

Unfortunately, there are two split packages:

- org.eclipse.jdt.internal.compiler
- org.eclipse.jdt.internal.compiler.parser

So the new bundle exports them to jdt.core and jdt.core re-exports.

org.eclipse.jdt.compiler.apt and org.eclipse.jdt.compiler.tool were
fragments of jdt.core, now they are inside
org.eclipse.jdt.core.compiler.batch.

TODO:

1) What I did NOT tried is to re-write all the magic scripts that build
and package separated ecj library out of jdt.core.
2) The 3 antadapter classes are now split over ecj and jdt core, because
BuildJarIndex.java and CheckDebugAttributes.java depend on JDT core
code.
3) ecj.1 and build_ecj.xml aren't touched yet. ecj.1 was touched last
time 2017, it seem to be used for man pages.
4) pom from jdt core will need an adoption.
5) org.eclipse.jdt-feature need to be updated
6) TBC

See
- eclipse-jdt#181
- eclipse-platform/eclipse.platform.ua#18
iloveeclipse added a commit to eclipse-jdt/eclipse.jdt.core that referenced this issue Dec 1, 2022
The org.eclipse.jdt.core.ecj.validation was a "dummy" bundle so far,
used only to validate compilation issues in IDE.

That one is renamed (org.eclipse.jdt.core.ecj.validation ->
org.eclipse.jdt.core.compiler.batch) and is a proper maven library now.

It is actually the ecj compiler library without any dependencies (except
optional ant), that could be consumed by JDT and the rest of the world.

It must be required and re-exported by JDT core.

Unfortunately, there are two split packages:

- org.eclipse.jdt.internal.compiler
- org.eclipse.jdt.internal.compiler.parser

So the new bundle exports them to jdt.core and jdt.core re-exports.

org.eclipse.jdt.compiler.apt and org.eclipse.jdt.compiler.tool were
fragments of jdt.core, now they are inside
org.eclipse.jdt.core.compiler.batch.

TODO:

1) What I did NOT tried is to re-write all the magic scripts that build
and package separated ecj library out of jdt.core.
2) The 3 antadapter classes are now split over ecj and jdt core, because
BuildJarIndex.java and CheckDebugAttributes.java depend on JDT core
code.
3) ecj.1 and build_ecj.xml aren't touched yet. ecj.1 was touched last
time 2017, it seem to be used for man pages.
4) pom from jdt core will need an adoption.
5) org.eclipse.jdt-feature need to be updated
6) TBC

See
- #181
- eclipse-platform/eclipse.platform.ua#18
@akurtakov
Copy link
Member Author

We are done here via 3a6e2d9

@Phillipus
Copy link

We are done here via 3a6e2d9

Tested our RCP app against 4.27.0.I20221206-1800 and can confirm this is working. Thanks!

iloveeclipse added a commit to eclipse-jdt/eclipse.jdt.core that referenced this issue Dec 7, 2022
The org.eclipse.jdt.core.ecj.validation was a "dummy" bundle so far,
used only to validate compilation issues in IDE.

That one is renamed (org.eclipse.jdt.core.ecj.validation ->
org.eclipse.jdt.core.compiler.batch) and is a proper maven library now.

It is actually the ecj compiler library without any dependencies (except
optional ant), that could be consumed by JDT and the rest of the world.

It must be required and re-exported by JDT core.

Unfortunately, there are two split packages:

- org.eclipse.jdt.internal.compiler
- org.eclipse.jdt.internal.compiler.parser

So the new bundle exports them to jdt.core and jdt.core re-exports.

org.eclipse.jdt.compiler.apt and org.eclipse.jdt.compiler.tool were
fragments of jdt.core, now they are inside
org.eclipse.jdt.core.compiler.batch.

TODO:

1) What I did NOT tried is to re-write all the magic scripts that build
and package separated ecj library out of jdt.core.
2) The 3 antadapter classes are now split over ecj and jdt core, because
BuildJarIndex.java and CheckDebugAttributes.java depend on JDT core
code.
3) ecj.1 and build_ecj.xml aren't touched yet. ecj.1 was touched last
time 2017, it seem to be used for man pages.
4) pom from jdt core will need an adoption.
5) org.eclipse.jdt-feature need to be updated
6) TBC

See
- #181
- eclipse-platform/eclipse.platform.ua#18
@eric-milles
Copy link

I think the Require-Bundle declaration of org.eclipse.jdt.core.compiler.batch should include a version range. As it stands, it binds to one specific version at packaging time. This prevents patching or updating JDT Core separately from the dependent feature(s). See groovy/groovy-eclipse#1425

@laeubi
Copy link
Contributor

laeubi commented Dec 17, 2022

I think the Require-Bundle declaration of org.eclipse.jdt.core.compiler.batch should include a version range. As it stands, it binds to one specific version at packaging time. This prevents patching or updating JDT Core separately from the dependent feature(s). See groovy/groovy-eclipse#1425

Do you like open a PR with your proposal?

@iloveeclipse
Copy link
Member

Do you like open a PR with your proposal?

I've created #78 to add require bundle version range for org.eclipse.help.webapp bundle, but I believe this alone would not help.

Reading through groovy/groovy-eclipse#1425 I assume the PR should be for https://projects.eclipse.org/projects/technology.packaging/ project which seem to still use gerrit for reviews, see https://git.eclipse.org/c/epp/org.eclipse.epp.packages.git/ / has no github repo.
@merks : I'm right?

If so, we need a change in org.eclipse.epp.package.common.feature/feature.xml that replaces require bundle from org.eclipse.jdt.core to org.eclipse.jdt.core.compiler.batch bundle, similar to 3a6e2d9

@merks
Copy link
Contributor

merks commented Dec 21, 2022

Yes, EPP is still using Gerrit. I created an Oomph setup for EPP a while back, so it's very easy to create an IDE for contributing to EPP...

@iloveeclipse
Copy link
Member

I created an Oomph setup for EPP a while back, so it's very easy to create an IDE for contributing to EPP...

Ed or @jonahgraham: may I ask you to update this feature file, should be easy to setup :-) ?

@merks
Copy link
Contributor

merks commented Dec 21, 2022

I already have one set up so yes, it's super easy for me to do that for you. But I wonder now exactly what is correct though...

I can of course easily change the common feature like this:

image

But this means that packages that install org.eclipse.jdt.core will also install org.eclipse.jdt.core.compiler.batch in this case because with this change all packages will install that. But I don't know if that's a redundant thing to install. So maybe we have to change each package that doesn't install org.eclipse.jdt.core to install org.eclipse.jdt.core.compiler.batch.

@iloveeclipse I assume it's redundant to install org.eclipse.jdt.core.compiler.batch and that installing just org.eclipse.jdt.core does not also install org.eclipse.jdt.core.compiler.batch. Is that a correct assumption or is it a stupid assumption?

@merks
Copy link
Contributor

merks commented Dec 21, 2022

I have a feeling that the changes made in the platform are sufficient already. E.g., the CDT package already installs org.eclipse.help.webapp and that now requires org.eclipse.jdt.core.compiler.batch so there is no need to specify that dependency elsewhere. I'll try removing it.

https://git.eclipse.org/r/c/epp/org.eclipse.epp.packages/+/197791

@iloveeclipse
Copy link
Member

I'll try removing it.

Yes, sure, this is the right thing to do, I somehow missed the fact the entire commit can be reverted.

@merks
Copy link
Contributor

merks commented Dec 22, 2022

I wonder if this issue is done now?

@iloveeclipse
Copy link
Member

I wonder if this issue is done now?

Why wondering, if you have merged the last PR :-) ?

@stanio
Copy link

stanio commented Dec 23, 2022

Will there be an update available for the Eclipse 4.26 (2022‑12) release?

@akurtakov
Copy link
Member Author

Will there be an update available for the Eclipse 4.26 (2022‑12) release?

No, but 4.26 should be working fine by requiring org.eclipse.jdt.core just few more extra dependencies bringed in.

@eric-milles
Copy link

4.26 should be working fine by requiring org.eclipse.jdt.core just few more extra dependencies

4.26 is not working fine for any feature that tries to patch the JDT Core bundle. See groovy/groovy-eclipse#1425

eclipsewebmaster pushed a commit to eclipse-packaging/packages that referenced this issue Mar 23, 2023
The org.eclipse.help.webapp bundle already specify the necessary
requirements for JSPs to compile.

eclipse-platform/eclipse.platform.ua#18

Change-Id: I35c36a68ee0e2b80cc4abe5656219e4501b05fce
robstryker pushed a commit to robstryker/eclipse.jdt.core that referenced this issue Jul 18, 2024
The org.eclipse.jdt.core.ecj.validation was a "dummy" bundle so far,
used only to validate compilation issues in IDE.

That one is renamed (org.eclipse.jdt.core.ecj.validation ->
org.eclipse.jdt.core.compiler.batch) and is a proper maven library now.

It is actually the ecj compiler library without any dependencies (except
optional ant), that could be consumed by JDT and the rest of the world.

It must be required and re-exported by JDT core.

Unfortunately, there are two split packages:

- org.eclipse.jdt.internal.compiler
- org.eclipse.jdt.internal.compiler.parser

So the new bundle exports them to jdt.core and jdt.core re-exports.

org.eclipse.jdt.compiler.apt and org.eclipse.jdt.compiler.tool were
fragments of jdt.core, now they are inside
org.eclipse.jdt.core.compiler.batch.

TODO:

1) What I did NOT tried is to re-write all the magic scripts that build
and package separated ecj library out of jdt.core.
2) The 3 antadapter classes are now split over ecj and jdt core, because
BuildJarIndex.java and CheckDebugAttributes.java depend on JDT core
code.
3) ecj.1 and build_ecj.xml aren't touched yet. ecj.1 was touched last
time 2017, it seem to be used for man pages.
4) pom from jdt core will need an adoption.
5) org.eclipse.jdt-feature need to be updated
6) TBC

See
- eclipse-jdt#181
- eclipse-platform/eclipse.platform.ua#18
robstryker pushed a commit to robstryker/eclipse.jdt.core that referenced this issue Jul 18, 2024
The org.eclipse.jdt.core.ecj.validation was a "dummy" bundle so far,
used only to validate compilation issues in IDE.

That one is renamed (org.eclipse.jdt.core.ecj.validation ->
org.eclipse.jdt.core.compiler.batch) and is a proper maven library now.

It is actually the ecj compiler library without any dependencies (except
optional ant), that could be consumed by JDT and the rest of the world.

It must be required and re-exported by JDT core.

Unfortunately, there are two split packages:

- org.eclipse.jdt.internal.compiler
- org.eclipse.jdt.internal.compiler.parser

So the new bundle exports them to jdt.core and jdt.core re-exports.

org.eclipse.jdt.compiler.apt and org.eclipse.jdt.compiler.tool were
fragments of jdt.core, now they are inside
org.eclipse.jdt.core.compiler.batch.

TODO:

1) What I did NOT tried is to re-write all the magic scripts that build
and package separated ecj library out of jdt.core.
2) The 3 antadapter classes are now split over ecj and jdt core, because
BuildJarIndex.java and CheckDebugAttributes.java depend on JDT core
code.
3) ecj.1 and build_ecj.xml aren't touched yet. ecj.1 was touched last
time 2017, it seem to be used for man pages.
4) pom from jdt core will need an adoption.
5) org.eclipse.jdt-feature need to be updated
6) TBC

See
- eclipse-jdt#181
- eclipse-platform/eclipse.platform.ua#18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.