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

Missing constraints in org.eclipse.jetty.* in latest I builds #779

Closed
Phillipus opened this issue Oct 4, 2023 · 24 comments
Closed

Missing constraints in org.eclipse.jetty.* in latest I builds #779

Phillipus opened this issue Oct 4, 2023 · 24 comments

Comments

@Phillipus
Copy link
Contributor

Phillipus commented Oct 4, 2023

Follow on from eclipse-equinox/p2#344

  1. Download latest I build Eclipse SDK from here - https://download.eclipse.org/eclipse/downloads/
  2. Launch Eclipse
  3. Open the "Target Platform State"
  4. Select "Show only unresolved plug-ins" option

errors

Launching a child Eclipse session also shows missing constraints errors and help system doesn't launch in child Eclipse.

@akurtakov
Copy link
Member

So issue comes from

return (((Integer) supplier.getDirective("x-equinox-ee")).intValue() > 0); //$NON-NLS-1$
which itself uses the old ExportPackageDescriptionImpl from o.e.osgi.compatibility.state.
My guess - As the package in question "java.lang.runtime" is since Java 14 for which there is no target file this old implementation fails to discover it as JVM package.
@HannesWell You planned to work on switching to the new resolver, right?

@Phillipus
Copy link
Contributor Author

But it's not just the StateViewPage. Launching a child Eclipse instance (or RCP app) against latest SDK target gives unresolved errors as well. (Unless I misunderstood last comment, apologies)

@HannesWell
Copy link
Member

HannesWell commented Oct 4, 2023

My guess - As the package in question "java.lang.runtime" is since Java 14 for which there is no target file this old implementation fails to discover it as JVM package.

I can confirm that the package only exists since Java-14 but for Java-9 or later BREEs the system packages are queried from the VM (and since the module system exists the JVM knows when a package was added).

But the Manifest of org.eclipse.jetty.server (and I assume other Jetty bundles are similar) declares a BREE of only JavaSE-11 although the migration guide mentions Java-17 as minimum required version and the mentionted package requires it to:

Bundle-RequiredExecutionEnvironment: JavaSE-11
Bundle-SymbolicName: org.eclipse.jetty.server
Bundle-Vendor: Eclipse Jetty Project

So PDE seems actually right here, but the metadata of Jetty seem to be wrong.

@HannesWell
Copy link
Member

HannesWell commented Oct 4, 2023

This seems to be the bad line:
https://github.com/eclipse/jetty.project/blob/476874584943daf3041eb0ca584d4de3a87f19fd/pom.xml#L845

If they remove that line and the following...
https://github.com/eclipse/jetty.project/blob/476874584943daf3041eb0ca584d4de3a87f19fd/pom.xml#L856

... the maven-bundle-plugin should add the EE requirement automatically based on the java version target (which should be Java-17):
https://github.com/eclipse/jetty.project/blob/476874584943daf3041eb0ca584d4de3a87f19fd/pom.xml#L16-L18

@akurtakov
Copy link
Member

I haven't thought about such possibility at all. I'll report it to Jetty.

@HannesWell
Copy link
Member

I haven't thought about such possibility at all. I'll report it to Jetty.

Great 👍🏽
If we are quick there is probably a chance to get it into the next release:
jetty/jetty.project#10649

@akurtakov
Copy link
Member

I still wonder though if pde queries the JVM and it runs on Java 17 why isn't it finding the package (as p2/equinox seems to be happy)?

@HannesWell
Copy link
Member

I still wonder though if pde queries the JVM and it runs on Java 17 why isn't it finding the package (as p2/equinox seems to be happy)?

Because PDE asks for the JVM for packages of the release the bundle declares as its BREE/EE-requirement. Equinox just uses the running VM and I assume P2 does the same, but I'm not sure about that.
The reason for the difference is that in the workspace one does not know which JRE is eventually used to run a framework with that bundle, this is only known when launching a product/application, but the best guess is the declared BREE.
There was some discussion about that in the end of #429, but since no approach is perfect (except running it) assuming the BREE seems the best guess, especially for java.* packages.
Otherwise it could happen that a Plugins resolves in the workspace but later fails at runtime (in different ways depending on the exact discrepancy).

And in this case the metadata are clearly wrong.

@HannesWell
Copy link
Member

HannesWell commented Oct 4, 2023

But it's not just the StateViewPage. Launching a child Eclipse instance (or RCP app) against latest SDK target gives unresolved errors as well. (Unless I misunderstood last comment, apologies)

The validation failure described in eclipse-equinox/p2#344 (comment) is probably a follow-up error because only resolved bundles are added automatically as requirements to a launch.
If the mentioned bundles are only included transitively it should work to add them explicitly to a launch as bundles or included in a feature that is explicitly listed as feature of a launch (or product).
The launch validation considers all packages of the VM used to run the app/product regardless of their version since:
https://github.com/eclipse-pde/eclipse.pde/pull/693/files#diff-4325b69067f5bdeed9111b3e511cdb233e4f11051502e280c33f79d8299940a6

@mickaelistria
Copy link
Contributor

While the Jetty case is strange, I don't think it can be claimed at bogus, and PDE should still support it:
The JavaSE-11 and Import-Package: java.lang.runtime combination is supported by any Java 14+ installation. I think that PDE (and maybe p2) understands here the BREE not as a plain requirement, that can be resolved by different versions Java installation, but as a constraint which locks the BREE and system packages.

@merks
Copy link
Contributor

merks commented Oct 5, 2023

I'm not sure how it can be anything but bogus to declare that one can run in Java 11 when one cannot. This will simply fail to wire the package and hence fail to start the bundle when running with Java 11. That to my thinking is about as bogus as it gets...

Whether PDE should tolerate such bogus things because the package constraint can in fact be satisfied by the actual JVM that is really used or will be really be used is different question than whether the combination of a BREE and a package requirement that does not exist for that BREE is bogus or not...

@mickaelistria
Copy link
Contributor

I'm not sure how it can be anything but bogus to declare that one can run in Java 11 when one cannot

That statement IMO illustrate that the interpretation of BREE is bogus: the bundle doesn't say it requires Java 11 to run.
Tt says it requires Java 11 support + Import-Package: java.lang.runtime. So practically, this usually resolves to Java 14 or later, but it also allows that if the java.lang.runtime is provided by another source, then this all can run on Java 11.

@laeubi
Copy link
Contributor

laeubi commented Oct 5, 2023

First of all BREE is deprecated (!) because it actually has no real semantics anymore on Modular (Java 9+) JVMs, so jetty should simply remove it and replace this by osgi.ee Namespace and import of java. packages (what jetty already does).

Nerveless using Java-11 where the classes are compiled with Java-17 is wrong and will, if trying to run on Java-11 OSGi framework result in class-version errors and is obviously not very useful, but is only a hint for the framework to not even try to start a bundle that requires a higher version of java (== byte code level!).

The available packages can vary, even on Java 17 VM I can build one that does not contain all packages I had ad runtime. That's why I previously already stated that PDE should only ever use ONE JVM for its target state and that's the target jvm (what is the default jvm of the workspace in practice), for a bundle in the workspace it should use the one declared in the project to prevent such errornous usages of packages not available at that JVM at all.

but it also allows that if the java.lang.runtime is provided by another source

java.* packages can only be provided by the JVM but not by a bundle for security reasons!

then this all can run on Java 11

even if, it can resolve but not run if the class version requires Java 17.

@merks
Copy link
Contributor

merks commented Oct 5, 2023

That it might get java.lang.runtime from another source might even be true if OSGi allowed one to export java.* packages, but it does not allow that. Moreover nothing else actually does provide that package in the actual target platform being used so the fact that something might do that in an alternative universe where bundles could provide java.* packages is not relevant. So in the end, nothing other than a Java 14+ JVM actually supplies or could supply this thing... I don't think your "it's not bogus" argument holds water, so it's not an effective argument as to why PDE should support this.

A more effective argument is that say that the user has made it clear to PDE that they are/will be actually launching with Java 17 so that package will actually resolve so please don't complain about it...

Anyway, you can see how the discussion about the bogusness of the constraint is not moving towards a resolution of PDE should support it...

@laeubi
Copy link
Contributor

laeubi commented Oct 5, 2023

Anyway, you can see how the discussion about the bogusness of the constraint is not moving towards a resolution of PDE should support it...

In the end it all boils don to how much PDE should validate external dependencies that are not build by PDE in the current workspace!

e.g. lets make it simpler and assume jetty would not be used as a bundle but a plain jar, it claims to run on Java 11 as it is compiled with -source=11 but using java 14 classes.
Now I have my project configured this as a dependency and using java 17, should user expect from JDT to complain that the dependency (if run on java 11) would not work and therefore is bogus / invalid / unusable / ... ?

The same is here, PDE can detect an issue here (that obviously should be fixed), but should it report it if I never asked for running the bundle with java 11 nor even compile my project with java 11 (so the project itself can never run on java 11 as well)?

@merks
Copy link
Contributor

merks commented Oct 5, 2023

Yes, these are much better statements. One might even argue that a java.* package can only come from a JRE so let's never complain about missing java.* package requirements. Or one might argue "thank goodness PDE complained so that this real problem is getting fixed".

@mickaelistria
Copy link
Contributor

I agree that PDE shouldn't validate the full bundle integrity like it does during development at that stage, but simply validate in launch config that the bundle can resolve with the current JRE and other bits of the environment.

@Phillipus
Copy link
Contributor Author

Phillipus commented Oct 5, 2023

But the Manifest of org.eclipse.jetty.server (and I assume other Jetty bundles are similar) declares a BREE of only JavaSE-11 although the migration guide mentions Java-17 as minimum required version and the mentionted package requires it to:

Bundle-RequiredExecutionEnvironment: JavaSE-11
Bundle-SymbolicName: org.eclipse.jetty.server
Bundle-Vendor: Eclipse Jetty Project

So PDE seems actually right here, but the metadata of Jetty seem to be wrong.

Yes, as an experiment I edited the MANIFEST.MF files inside of the Jetty jars (org.eclipse.jetty.util_12.0.1.jar, org.eclipse.jetty.session_12.0.1.jar, org.eclipse.jetty.io_12.0.1.jar, org.eclipse.jetty.http_12.0.1.jar, org.eclipse.jetty.server_12.0.1.jar) and changed it to JavaSE-17 and everything resolved OK.

@akurtakov
Copy link
Member

Jetty 12.0.2 should contain a fix for that and it should be released soon so we can overcome this issue. I think it's easier to live with this issue (for short period) instead of revert and reapply in all the involved git repos.

@HannesWell
Copy link
Member

I agree that PDE shouldn't validate the full bundle integrity like it does during development at that stage, but simply validate in launch config that the bundle can resolve with the current JRE and other bits of the environment.

It is not that simple since there is no explicit validation on this topic for internal or external Bundles. What's happening here is that the bundle simply does not resolve in the org.eclipse.osgi.service.resolver.State of the PDEState, because the EE of the Bundles does not have the imported package available.
In general it would be possible to provide only the EE of the 'Target-VM' with its packages in the state, which would have the effect that also lower EE requirements are resolved to that (at the moment there is one EE provide for each java version) and would only see the packages of the one 'Target-VM'.
But that would then also be applied for Plugin projects in the workspace. As far as I know there is also no possibility to distinguish in the State since it only know Bundles, regardless of their origin.

And even if it would be possible, should we really silence the only tool that indicated that error? Ideally BND would already detect such muss configuration, but unfortunately it didn't. I'd rather fix the issue and keep the more strict (implicit) validation. Until know I have not seen a relevant case where it was wrong.

I also want to emphasize that, in the pre-launch validation the system-packages only of the JVM (at its own Java-version) used to run the launch are provided to all bundles regardless of their EE-requirement. So there you already have the desired behavior.
That Plugins are still missing usually comes from the fact that unresolved Bundles are not added automatically as transitive dependencies.

@laeubi
Copy link
Contributor

laeubi commented Oct 7, 2023

the State since it only know Bundles, regardless of their origin

each bundle has a (install) location where you can se where it is located and could then be matched to a workspace project if needed.

@Phillipus Phillipus changed the title Missing contraints in org.eclipse.jetty.* in latest I builds Missing constraints in org.eclipse.jetty.* in latest I builds Oct 7, 2023
@Phillipus
Copy link
Contributor Author

Phillipus commented Oct 10, 2023

I think Jetty 12.02 has now been released.

What needs to happen now to ship Jetty 12.02 in the platform build? (Please note, I am only asking out of curiosity, not urgency :-) )

Edit: please ignore, I found this eclipse-platform/eclipse.platform.releng.aggregator#1430

Thanks!

@Phillipus
Copy link
Contributor Author

I've tested against latest I build (20231010-2112) and this issue seems to be resolved.

Shall I close this issue?

@Phillipus
Copy link
Contributor Author

Thanks to everybody who worked on this issue! 👍

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

No branches or pull requests

6 participants