Replace aether-okhttp-connector with Maven's wagon-http

[fbricon]

Fixes #1510

Change-Id: Ic76a241f29a2a37f63d9f4c9b5829067248c9767
Signed-off-by: Fred Bricon fbricon@gmail.com

[fbricon]

Manual testing worked fine. Now let's see how those pesky unit tests will do

[github-actions]

Test Results

107 files  ±0  107 suites  ±0   6m 41s ⏱️ - 4m 49s
662 tests ±0  652 ✔️ +1  10 💤 ±0  0 ❌  - 1 
662 runs  ±0  651 ✔️ +1  11 💤 ±0  0 ❌  - 1 

Results for commit 006ddda. ± Comparison against base commit 43425f1.

♻️ This comment has been updated with latest results.

[HannesWell]

The build failed with the following message:

[ERROR] Failed to execute goal org.eclipse.tycho:tycho-p2-plugin:4.0.1:p2-metadata (p2-metadata) on project org.eclipse.m2e.feature: baseline and build artifacts have same version but different contents
 [ERROR]    no-classifier: different
 [ERROR]       feature.xml: different
 [ERROR]    classifier-sources-feature: different
 [ERROR]       feature.xml: different
 [ERROR] -> [Help 1]
 org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.eclipse.tycho:tycho-p2-plugin:4.0.1:p2-metadata (p2-metadata) on project org.eclipse.m2e.feature: baseline and build artifacts have same version but different contents
    no-classifier: different
       feature.xml: different
    classifier-sources-feature: different
       feature.xml: different

You probably have to enforce a new qualifier version in the feature like in https://github.com/eclipse-m2e/m2e-core/pull/1370/files#diff-75c2153a38dd2bbf3e3e2e587713e98a5f10f214e65544620fc6f13eb70fce19

[HannesWell]

I just checked the content difference of m2e.maven.runtime with this change.
B is the new version with this PR, A is the current snapshot. Green means present, while black is absent. Red means different (which is only jansi due to missing mac signatures).

[HannesWell]

This increases the size of m2e.maven.runtime from 7.8MB to 8.6MB (without signatures).
I'll have a look if we can import some of the dependencies of wagon-http.

[HannesWell]

This increases the size of m2e.maven.runtime from 7.8MB to 8.6MB (without signatures). I'll have a look if we can import some of the dependencies of wagon-http.

@fbricon I have pushed another commit that excludes the httpcomponents from the m2e.maven.runtime bundle and just imports the packages.
Eventualy this should be squashed into your commit.

[fbricon]

Any idea why there are 2 versions of httpcore and httpclient? I tried cleaning target/ + updating the project several times, but no dice.

[HannesWell]

Any idea why there are 2 versions of httpcore and httpclient? I tried cleaning target/ + updating the project several times, but no dice.

I had a similar problem and cleaned it multiple times. In the end, only a restart fixed it. :/

[HannesWell]

@fbricon I finally managed to fix the mistake in #1370 and completed the upgrade to Maven 3.9.4. The version-bumps therefore should not be necessary anymore.

Do you plan to finish this any-time soon? I plan to release m2e 2.4.0 in the beginning of next week just in time for SimRel 2023-09 RC1. Therefore I would find it saver to merge this after the release so that there is more time for testing, since this is a relatively fundamental change in the maven.runtime, even if this is ready before.

[fbricon]

I'll give it another try tomorrow, but will keep wagon in the runtime, to stay consistent with Maven itself.

[kwin]

Maven switched to HTTP Native for resolver recently and ditched Wagon Resolver: https://maven.apache.org/docs/3.9.0/release-notes.html. Is there any reason why m2e should still leverage Wagon?

[fbricon]

@fbricon I have pushed another commit that excludes the httpcomponents from the m2e.maven.runtime bundle and just imports the packages. Eventualy this should be squashed into your commit.

I've removed the httpcomponents imports from the TP, as the Eclipse update site no longer provides httpclient but httpclient5 ( with different package names), so I'm letting Maven Runtime taking the responsibility of embedding the jars.

I kept wagon-http in, as it's easier to migrate some tests (the infamous HttpxWagon), but I checked overall dependency resolution uses maven-resolver-transport-http, as it takes precedence over wagon since 3.9

[fbricon]

Why is lemminx.tests the only plugin showing test failures with:

org/apache/maven/wagon/providers/http/HttpWagon
java.lang.NoClassDefFoundError: org/apache/maven/wagon/providers/http/HttpWagon
	at java.base/java.lang.ClassLoader.defineClass1(Native Method)
	at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1017)
	at org.eclipse.osgi.internal.loader.ModuleClassLoader.defineClass(ModuleClassLoader.java:283)
	at org.eclipse.osgi.internal.loader.classpath.ClasspathManager.defineClass(ClasspathManager.java:716)
	at org.eclipse.osgi.internal.loader.classpath.ClasspathManager.findClassImpl(ClasspathManager.java:639)
	at org.eclipse.osgi.internal.loader.classpath.ClasspathManager.findLocalClassImpl(ClasspathManager.java:607)
	at org.eclipse.osgi.internal.loader.classpath.ClasspathManager.findLocalClassImpl(ClasspathManager.java:587)
	at org.eclipse.osgi.internal.loader.classpath.ClasspathManager.findLocalClass(ClasspathManager.java:566)
	at org.eclipse.osgi.internal.loader.ModuleClassLoader.findLocalClass(ModuleClassLoader.java:335)
	at org.eclipse.osgi.internal.loader.BundleLoader.findLocalClass(BundleLoader.java:397)
	at org.eclipse.osgi.internal.loader.BundleLoader.findClass0(BundleLoader.java:500)
	at org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java:416)
	at org.eclipse.osgi.internal.loader.ModuleClassLoader.loadClass(ModuleClassLoader.java:168)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)
	at org.eclipse.m2e.tests.common.AbstractMavenProjectTestCase.setUp(AbstractMavenProjectTestCase.java:248)

What makes is a special snowflake? Plenty of other tests, extending AbstractMavenProjectTestCase, in other plugins pass

[HannesWell]

What makes is a special snowflake? Plenty of other tests, extending AbstractMavenProjectTestCase, in other plugins pass

Not really an idea. They probably start a language server, but the stack-trace does not involve that. So no clue.

[mickaelistria]

The log at https://ci.eclipse.org/m2e/job/m2e/job/PR-1513/19/artifact/org.eclipse.m2e.editor.lemminx.tests/target/work/data/.metadata/.log seems to indicate that there are circular references.

[HannesWell]

@fbricon in a second commit I removed the Apache commons-lang3/codec from the embedded jars and instead added them to the target-platform, which saves almost one MB in unzipped state.

The list of jars in m2e.maven.runtime now looks much more like in a standard maven distribution, which I think is good.
Given that all tests pass I would be fine with this change as it is.
Nevertheless I think this replacement should be mentioned in the Release-notes because for m2e-connectors this could be a breaking change, if they relayed on okhttp.

[fbricon]

@HannesWell I updated the release notes

[fbricon]

I believe the EditorTest.testOpenChildThenParentResolvesParent test failure might be caused by lemminx-maven requiring okhttp

Remote search definitely doesn't work, but I see no errors in the lemminx logs

[HannesWell]

@HannesWell I updated the release notes

Thanks, it great to have such elaborative notes.
In order to ease migration for those affected by the removal, maybe a link the the Guide Mentioned by Tamas in his answers in #1510 (comment) would be useful?
https://maven.apache.org/guides/mini/guide-resolver-transport.html

I believe the EditorTest.testOpenChildThenParentResolvesParent test failure might be caused by lemminx-maven requiring okhttp

I thought about that as well and indeed LemMinX depends on takari okhttp:

https://github.com/eclipse/lemminx-maven/blob/c42b5552be9df087db2660807589d9bfd622fbf5/lemminx-maven/pom.xml#L134-L136

https://github.com/eclipse/lemminx-maven/blob/c42b5552be9df087db2660807589d9bfd622fbf5/lemminx-maven/src/main/java/org/eclipse/lemminx/extensions/maven/searcher/RemoteCentralRepositorySearcher.java#L55-L59

I think that needs to be migrated first to the new resolver released first and then m2e needs to be updated to the new LemMinX in this PR too?
The question is, can you respectively @vrubezhny do that until Tuesday? Then I wanted to release m2e 2.4 for SimRel 2023-09.
For me personally there is no need to rush and I would be fine to postpone this and merge it in the beginning of the next development cycle. This would also give us more time to catch potential regressions.

[HannesWell]

Maven 3.9.5 is currently on vote, therefore it would be cheap in terms of version bumps to finish this before the next m2e release (probably in time for SimRel 2023-12).

[mickaelistria]

FWIW, lemminx-maven now uses Maven 3.9.5 as API in snapshots (without any change it its code since 3.9.4)

[HannesWell]

FWIW, lemminx-maven now uses Maven 3.9.5 as API in snapshots (without any change it its code since 3.9.4)

But as far as I can tell it also uses the okhttp API provided by m2e. Or are the following already fixed/obsolete?

• Replace aether-okhttp-connector with Maven's wagon-http eclipse-lemminx/lemminx-maven#508
• Get rid of aether-okhttp-connector #508 eclipse-lemminx/lemminx-maven#509

[mickaelistria]

Nothing changed in lemminx-maven per se. But maybe now with newer Maven, PR eclipse-lemminx/lemminx-maven#509 can be made to work?

[HannesWell]

@fbricon since lemminx-maven 0.11 is now available since #1599 I rebased this PR, resolved the conflicts and squashed the commits.
I also tried locally to reference the httpclient and httpcore jars as dependencies instead of embedding them, but discarded that since they are only provided by Orbit and the osgi bundle has commons-codec embedded but in an older version that has CVEs, which is why maven-resolver-transport-http has a more recent dependency on it to replace the jar although the code does not need it: https://github.com/apache/maven-resolver/pull/320/files
But for httpclient-4 the dependency is not updated due to missing Java 1.6 support:
apache/httpcomponents-client#411

If the build passes I'm fine with this change.
What do you or the others think?

If this should land for the december SimRel I want to land this soon to make (a fixed) M3 contribution for m2e before the 2.5 release.

[fbricon]

tests are borked.

Exception java.lang.NoClassDefFoundError: org/apache/commons/lang3/math/NumberUtils [in thread "Worker-0: Repository registry initialization"]
at org.apache.maven.artifact.versioning.DefaultArtifactVersion.tryParseInt(DefaultArtifactVersion.java:181)

[HannesWell]

tests are borked.

Exception java.lang.NoClassDefFoundError: org/apache/commons/lang3/math/NumberUtils [in thread "Worker-0: Repository registry initialization"]
at org.apache.maven.artifact.versioning.DefaultArtifactVersion.tryParseInt(DefaultArtifactVersion.java:181)

Probably the imports have to be adjusted.
Will look into this this evening.

Besides that are you fine with it as it is?

[fbricon]

If you've manually tested that everything still works, including the XML editor, then yes

[HannesWell]

If you've manually tested that everything still works, including the XML editor, then yes

I have tested it a bit in a debugged Eclipse and didn't noticed any issue. Testing everything is of course impossible, but since the build is also green I don't expect severe issues. Any way I think the best test is to use it.
Therefore I'll submit this so that it can be used in the snapshots and I ask everyone here to update to the latest snapshot and use/test it. If I'm not aware about any issue until Wednesday I'll create an m2e-M3 snapshot and contribute it to simrel so that we have a greater audience to test.