Add lifecycle mapping for maven-toolchains-plugin #1637

[G-Ork]

For #1637

[HannesWell]

Thank you for this PR, the general intend looks good, but the lifecycle mappings should not be added to the m2e.apt.core plugin (see the comments).

[HannesWell]

Thank you for this contribution.
Looks good now. Lets wait for the result of the CI.

[github-actions]

Test Results

107 files  ±0  107 suites  ±0   6m 44s ⏱️ -4s
662 tests ±0  652 ✅ ±0  10 💤 ±0  0 ❌ ±0 
662 runs  ±0  651 ✅ ±0  11 💤 ±0  0 ❌ ±0 

Results for commit 298b181. ± Comparison against base commit 98dbefe.

♻️ This comment has been updated with latest results.

[HannesWell]

@G-Ork it looks like this causes new test-failures. Could you have a look at them?

[G-Ork]

I will. Seemed to be code thats generated somehow. Can not find the test classes mentioned.

[HannesWell]

I will. Seemed to be code thats generated somehow. Can not find the test classes mentioned.

These tests live in another repository, i.e. https://github.com/tesla/m2e-core-tests/, which is included as sub-module of this repository. Unfortunately, for legal reasons we are not allowed to add these tests to this repository. :/

The contribution guide explains how to do that manually: https://github.com/eclipse-m2e/m2e-core/blob/master/CONTRIBUTING.md#%EF%B8%8F-setting-up-the-development-environment-manually
or you just use the provided Oomph setup.

Anyway, it looks you already found a hot candidate for being the cause of the failures. 🚀

[G-Ork]

Is the check Build M2Eclipse / build (macos-latest) (pull_request) related to m2e-core-tests or is this another problem? Looks like it was executed before my last fixes and not again afterwards.

[HannesWell]

Is the check Build M2Eclipse / build (macos-latest) (pull_request) related to m2e-core-tests or is this another problem? Looks like it was executed before my last fixes and not again afterwards.

This is unrelated, to your change. All tests look good now. 👍🏽

Could you please just revert the version bumps in org.eclipse.m2e.core/META-INF/MANIFEST.MF and org.eclipse.m2e.core/pom.xml. This plugin's version has already been bumped since the last release so bumping it again should not be necessary.
Then this PR is ready for submission.

[G-Ork]

Thank you for the patience. I have removed the version bumps.
I've read it but did not check it 🤦‍♂️

[HannesWell]

Thank you for the patience. I have removed the version bumps. I've read it but did not check it 🤦‍♂️

No problem, you are welcome.
Thanks again for this contribution.

[laeubi]

@G-Ork now we have even UI for toolchans do you like to look into why ~/.m2/toolchains.xml is not found in the embedded context of m2e?

In maven CLI this happens with org.apache.maven.cli.MavenCli.toolchains(CliRequest) and is just a matter of org.apache.maven.execution.MavenExecutionRequest.setGlobalToolchainsFile(File) / org.apache.maven.execution.MavenExecutionRequest.setUserToolchainsFile(File) but m2e seems to not perform this currently in org.eclipse.m2e.core.internal.embedder.MavenExecutionContext.createExecutionRequest(IMavenConfiguration, IComponentLookup, Settings) this would be much more appropriate than disable the execution of the mojo.

[HannesWell]

but m2e seems to not perform this currently in org.eclipse.m2e.core.internal.embedder.MavenExecutionContext.createExecutionRequest(IMavenConfiguration, IComponentLookup, Settings) this would be much more appropriate than disable the execution of the mojo.

Actually this was implement as part of the PR that added the UI:
https://github.com/eclipse-m2e/m2e-core/pull/1643/files#diff-796a271293c65b5cac9c04a4dccddff344e43fd10f474a5cd2992c2333325704

and is now performed at

m2e-core/org.eclipse.m2e.core/src/org/eclipse/m2e/core/internal/embedder/MavenExecutionContext.java

Lines 211 to 214 in 53c0eaf

	if(mavenConfiguration.getUserToolchainsFile() != null) {
	userToolchainsFile = new File(mavenConfiguration.getUserToolchainsFile());
	}
	request.setUserToolchainsFile(userToolchainsFile);

So should we consider reverting this? @G-Ork what's your opinion about that?

[G-Ork]

Hi, i am currently preparing a PR with unit tests as asked for the change of the UI. The tests are finished and where necessary because the settings where not applied to the maven launcher. My fault. My Plan was to create the PR for the UI and the unit tests. The PR Should be ready today or at least this weekend.

After this i would dig into the open questions about the internal launch. This relate to the code you quoted. My opinion is that we should try to make it work. Or make it work somehow. I stumbled already over the code that configures different JVMs based on the enforcer plugin. That feature was added last year. I didn't test it yet but i guess i will help.

In my opinion this is quite hacky as the enforcer plugin is a assertion plugin and not for configuration. I was surprised as another developer mention that. Also found inside the hidden releasenotes. I would prefer a toolchain approach. I also run into this requirement this week by trying to migrate a project to java 21. Eclipse used the start JRE and i wasn't able to build with the internal build. Workaround was to use the Maven launcher and configure a 21 JDK.