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

Don't extract referenced bundles for Maven runtime classpath #926

Merged
merged 1 commit into from
Oct 1, 2022

Conversation

HannesWell
Copy link
Contributor

@HannesWell HannesWell commented Sep 22, 2022

Fixes #923

OSGi bundles that are referenced by the m2e.maven.runtime bundle don't
have to be extracted when added to the classpath of an about to be
launched Maven build. Not extracting them significantly speeds up the
launch of the first build after IDE start or opening the first time the
Launch-Config dialog for Maven builds or the Maven Installation
settings.

Additionally ensure that slf4j from Maven-Central is used instead of the
one from Orbit to prevent errors like the following when launching a
Maven-build with the embedded runtime from the IDE:

class "org.slf4j.MavenSlf4jFriend"'s signer information does not match
signer information of other classes in the same package

The reason for such an error is that the slf4j variant from Orbit is
jar-signed by Eclipse, while 'slf4j' and 'maven-slf4j-provider' from
Maven-Central is only PGP-signed (and not jarsigned). But because
'slf4j' and 'maven-slf4j-provider' provide classes for the package
org.slf4j they must provide the same signer information. This check is
not performed if a jar is extracted and added to the classpath as
directory, but we don't want to do that anymore.

slf4j-from Maven-central is enforced by specifing a lower bound of
1.7.31 for slf4j bundles, while Eclipse-Orbit only provides 1.7.30.

@HannesWell
Copy link
Contributor Author

Unfortunately with this solution launching a maven build fails with:

[ERROR] Error executing Maven.
java.lang.SecurityException: class "org.slf4j.MavenSlf4jFriend"'s signer information does not match signer information of other classes in the same package
	at java.base/java.lang.ClassLoader.checkCerts(ClassLoader.java:1158)
	at java.base/java.lang.ClassLoader.preDefineClass(ClassLoader.java:902)
	at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1010)

This is because the org.slf4j.api bundle comes from Orbit and is signed with the EF key, while the binding (the maven.sl4j.simple) comes from Maven-Central and is only PGP signed and not jar signed.
So in order to make this work we have to migrate M2E and Eclipse to use SLF4J+Logback from Maven Central. For M2E this is not a big deal, but we have to ensure that SimRel+EPP uses the Maven-Central variant as well.

I prepared a patch to do the migration for M2E already but that revealed that with logback-classic 1.2.11 (the last version that does not require slf4j 2) plus slf4j-api 1.7.36 the logback jars are also pulled in into a launched Maven build classpath because the 'connection' in the OSGi world is done via a org.slf4j.impl package that a binding has to implement. Consequently two SLF4J bindings are then present and a corresponding error message is displayed.
That would be avoided if M2E and the rest of the Eclipse Platform would migrate to SLF4J 2.0 and Logback 1.4 or 1.3 (the latter uses javax, while the former uses jakaratEE), because there the Service Loader approach is used.

Coincidentally I wanted to propose the latter anyways and collected the items I think that have to be done, but this requires some coordinated effort for all projects that use SLF4J. I plan to create a corresponding proposal/issue for Eclipse Platform at the weekend and maybe also post some hint at the cross-project mailing list.

@github-actions
Copy link

github-actions bot commented Sep 22, 2022

Unit Test Results

596 tests   590 ✔️  8m 11s ⏱️
  94 suites      6 💤
  94 files        0

Results for commit 266f4ff.

♻️ This comment has been updated with latest results.

@mickaelistria
Copy link
Contributor

IIRC the bundle symbolicName are different for Orbit vs upstream SLF4J, so if you use a Require-Bundle, then there would be no risk of getting the wrong one, even in SimRel.

@laeubi
Copy link
Member

laeubi commented Sep 23, 2022

This significantly speeds up opening the first time the Launch-Config dialog for Maven builds or the Maven Installation settings.

But maybe this then slows down other parts where this is used? Maybe we simply should not call the critical code section in the UI thread?

@HannesWell
Copy link
Contributor Author

HannesWell commented Sep 24, 2022

IIRC the bundle symbolicName are different for Orbit vs upstream SLF4J, so if you use a Require-Bundle, then there would be no risk of getting the wrong one, even in SimRel.

That's right, but it is a little bit more to consider. I have created a umbrella issue in eclipse-platform aggreagtor to discuss and coordinate more effort in that direction:
eclipse-platform/eclipse.platform.releng.aggregator#588
I invite everybody interested to participate.

This significantly speeds up opening the first time the Launch-Config dialog for Maven builds or the Maven Installation settings.

But maybe this then slows down other parts where this is used? Maybe we simply should not call the critical code section in the UI thread?

I don't expect slowdowns in other areas due to this change.
As far as I can tell the jars are only added to the classpath of a launched Maven-build and to read the embedded Maven version.
Since it pretty common to use jars on a classpath I assume that classloaders don't load faster from directories than from jars. At least a default Maven-installation also uses jars.
And reading the embedded Maven version in MavenEmbeddedRuntime.getVersion() also works very fast with a jar because it just opens a JarFile and looks for a specific entry. When I tested it there was no noticeable delay (actually it was noticeably speed up because the extraction is now not done anymore when initializing the classpath).

Regarding the slf4j issue:
I drafted PR #931 to migrate m2e's build to logback 1.2.11 from Maven-Central that should allow us to continue with this one without migrating to slf4j 2, which could take some time because it could require many changes even in third party projects.
However even the migration to logback 1.2.11 from Maven-Central has some risks as elaborated in the issue mentioned above. Therefore it should be discussed in a broader audience first.

@HannesWell HannesWell force-pushed the fix923 branch 12 times, most recently from 8170e42 to 21d11e8 Compare October 1, 2022 10:37
@HannesWell HannesWell force-pushed the fix923 branch 2 times, most recently from 183fa54 to e6f8afc Compare October 1, 2022 14:24
.gitmodules Outdated Show resolved Hide resolved
Fixes eclipse-m2e#923

OSGi bundles that are referenced by the m2e.maven.runtime bundle don't
have to be extracted when added to the classpath of an about to be
launched Maven build. Not extracting them significantly speeds up the
launch of the first build after IDE start or opening the first time the
Launch-Config dialog for Maven builds or the Maven Installation
settings.

Additionally ensure that slf4j from Maven-Central is used instead of the
one from Orbit to prevent errors like the following when launching a
Maven-build with the embedded runtime from the IDE:
```
class "org.slf4j.MavenSlf4jFriend"'s signer information does not match
signer information of other classes in the same package
```
The reason for such an error is that the slf4j variant from Orbit is
jar-signed by Eclipse, while 'slf4j' and 'maven-slf4j-provider' from
Maven-Central is only PGP-signed (and not jarsigned). But because
'slf4j' and 'maven-slf4j-provider' provide classes for the package
org.slf4j they must provide the same signer information. This check is
not performed if a jar is extracted and added to the classpath as
directory, but we don't want to do that anymore.

slf4j-from Maven-central is enforced by specifing a lower bound of
1.7.31 for slf4j bundles, while Eclipse-Orbit only provides 1.7.30.
@HannesWell
Copy link
Contributor Author

Great, it finally works. I also tested it in Eclipse launched from m2e-dev Eclipse.
The main problem was that the m2e.maven.runtime was not wired to the slf4j from Maven-Central but from Orbit, since both are currently in the TP. To enforce it I had to lift the lower bound of the package imports and made the package-import non optional. I don't remember anymore why I changed that in #282, but it is clearly required now so it can't be optional. I think it only worked before because other m2e plug-ins required the same slf4j version. But since the m2e.maven.runtime is now the driver it has to be more strict.

@HannesWell HannesWell merged commit 143c182 into eclipse-m2e:master Oct 1, 2022
@HannesWell HannesWell deleted the fix923 branch October 1, 2022 16:42
@HannesWell HannesWell added this to the 2.1.0 milestone Oct 22, 2022
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 this pull request may close these issues.

"Installations" preference freezes the UI for a number of seconds or longer
3 participants