Only evaluate major and minor version of enforcer java rule #981

kwin · 2022-10-11T12:35:59Z

Fix evaluation of expressions in m-enforcer-p parameter Only select ExecutionEnvironments with at least one compatible VM installed.
This closes #842

github-actions · 2022-10-11T13:19:01Z

Unit Test Results

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

Results for commit 57f75fd.

♻️ This comment has been updated with latest results.

HannesWell

In general that looks reasonable, but I have a general remark about the API enhancement.

Furthermore could you please add a test-case to ensure the associated issue is fixed.

org.eclipse.m2e.core/src/org/eclipse/m2e/core/internal/embedder/MavenImpl.java

org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/AbstractJavaProjectConfigurator.java

org.eclipse.m2e.core/src/org/eclipse/m2e/core/embedder/IMaven.java

kwin · 2022-11-07T16:50:26Z

org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/AbstractJavaProjectConfigurator.java

-      return null;
+  private String getMinimumJavaBuildEnvironmentId(MavenProject mavenProject, MojoExecution mojoExecution,
+      IProgressMonitor monitor) throws InvalidVersionSpecificationException, CoreException {
+    MojoUtils mojoUtils = new MojoUtils(maven, ((MavenImpl) maven).getPlexusContainer().getContainerRealm(),


@laeubi Any idea how to access the classLoader to use from outside without doing potentially invalid casting??

It depends on what class you want to access, do you have a specific error/problem here?

Everything works, but the pattern ((MavenImpl) maven).getPlexusContainer().getContainerRealm() assumes that the service IMaven is always implemented by MavenImpl. Is it ok to to push getPlexusContainer() to IMaven?

Actually using the plexus container should not be required at all, this is a bit an anti-pattern. Next, using the IMaven should be considered a potential code-smell, you are configuring a project here, so most likely you don't want the global context but a project context.

Please have a closer look at the code. I need a ClassLoader for conversion/expression interpolation. That was the getPlexusContainer().getContainerRealm() before. I don't want to change everything in this PR (you can do that subsequently) but to extract the Mojo value calculation to a dedicated class this is necessary.

Then it might be better to implement it in your plugin directly, m2e already contains some ugly hacks and we should not add another one. Using the global context and the container realm most likely giving wrong results in some setups that are hard to track.

in your plugin directly…

The issue and also this PR fixes an issue in m2e. But this kind of discussion doesn’t really foster contributions. Please don’t require bugfixes to improve everything. It is easy to follow up with a subsequent PR to make it a proper API. Since you committers couldn’t agree on an API please don’t let external contributors suffer from it.

Well you asked what the proper way would be to access the classloader and I told you that the current approach is probably broken. So it does not help to "fix" broken code with some other broken code. If you think it is better fixed with separate PRs that fine. Its often the case that considerable amount of time and effort has to be done before the actual issue can be fixed, and the main goal is that it works under all circumstances we currently have.

This is not "improve everything", but these changes are already beyond trivial single one-liners and thus likely deserve closer review what might reveal related issues to be fixed for a solid soloution.

While I agree with Christoph in general that if something changes it is better done right, I also see that it would take a non trivial extra amount of work and time for this issue and understand you Konrad that you don't want to do that for this bug fix and this got a bit out of hand.

So in order to speed up the fix of the associated bug we should accept a solution that is as bad as before in regards to choosing the 'right' classloader.

So @kwin could you 'reset' this PR to the state before you extracted the configuration handling to a new class and just a add a new method getMojoParameterValue(MavenProject project, MojoExecution mojoExecution, List<String> parameterPath, Class<T> asType, IProgressMonitor monitor) to MavenImpl, but not to IMaven.

We can then build a robust new API in #996, without pressure and also apply it to this location.

Ok, done now.

github-actions · 2022-11-07T16:58:59Z

Test Results

607 tests ±0 600 ✔️ ±0 8m 19s ⏱️ - 1m 22s
  97 suites ±0     7 💤 ±0
  97 files ±0     0 ❌ ±0

Results for commit 6d0acbf. ± Comparison against base commit d07bc99.

♻️ This comment has been updated with latest results.

kwin · 2022-11-25T13:40:27Z

Furthermore could you please add a test-case to ensure the associated issue is fixed.

Unfortunately not easily due to big amount of test failures with Mac OS.

HannesWell · 2022-11-25T18:46:38Z

Furthermore could you please add a test-case to ensure the associated issue is fixed.

Unfortunately not easily due to big amount of test failures with Mac OS.

Have you tried to create a new test case for this? Does that fail? You can simply ignore the failing tests on your computer for now. As long as the new test passes in the CI that's fine.

And could you please just squash all your changes to one commit with suitable message. This hopefully also makes the eclipsefdn-eca check pass.

If we manage to complete this change over the weekend I'm willing to wait for this so that it will be included in the next bug-fix release and eventually Eclipse 2022-12.

org.eclipse.m2e.feature/feature.xml

Fix evaluation of expressions in m-enforcer-p parameter Only select ExecutionEnvironments with at least one compatible VM installed. This closes eclipse-m2e#842 Also-by: Hannes Wellmann <wellmann.hannes1@gmx.net>

HannesWell

Given that I want to perform another bug-fix release tomorrow and that the discussion slowed this down I have added the tests by myself for this one time.
For the next time please provide a test case that covers the change. Fixes without tests are only half the battle, because without them you can't ensure that the issue does not happen again in the future.

Furthermore I reworked the fix itself a bit.
@kwin check if this is still correct.

@laeubi, @mickaelistria do you want to review this as well?

laeubi · 2022-11-28T05:22:37Z

@HannesWell As you have taken a deeper look on this and my previous concerns are addressed I would leave it up to you how to proceed here.

HannesWell · 2022-11-28T17:57:53Z

@HannesWell As you have taken a deeper look on this and my previous concerns are addressed I would leave it up to you how to proceed here.

Alright, then I think this is good to go.
Thanks.

kwin mentioned this pull request Oct 11, 2022

Java version mismatch between JRE_CONTAINER and targetPlatform in M2E 2.0.0 #842

Closed

kwin force-pushed the bugfix/improve-execution-id-configuration branch from e9bb4a3 to de53541 Compare October 11, 2022 12:39

kwin marked this pull request as ready for review October 11, 2022 13:40

HannesWell requested changes Oct 13, 2022

View reviewed changes

HannesWell mentioned this pull request Oct 18, 2022

Improve accessibility of Mojo configuration parameters and (partially) use it in M2E #996

Draft

kwin force-pushed the bugfix/improve-execution-id-configuration branch from 57f75fd to 36e1c4a Compare November 7, 2022 16:47

kwin commented Nov 7, 2022

View reviewed changes

kwin force-pushed the bugfix/improve-execution-id-configuration branch 2 times, most recently from 2cb839d to 78550dc Compare November 25, 2022 13:38

kwin force-pushed the bugfix/improve-execution-id-configuration branch 2 times, most recently from 1786574 to 3fb03fa Compare November 27, 2022 10:43

HannesWell reviewed Nov 27, 2022

View reviewed changes

org.eclipse.m2e.feature/feature.xml Outdated Show resolved Hide resolved

kwin force-pushed the bugfix/improve-execution-id-configuration branch from 3fb03fa to 5fa26cf Compare November 27, 2022 10:58

Only evaluate major and minor version of enforcer java rule

6d0acbf

Fix evaluation of expressions in m-enforcer-p parameter Only select ExecutionEnvironments with at least one compatible VM installed. This closes eclipse-m2e#842 Also-by: Hannes Wellmann <wellmann.hannes1@gmx.net>

HannesWell force-pushed the bugfix/improve-execution-id-configuration branch from 5fa26cf to 6d0acbf Compare November 27, 2022 22:46

HannesWell approved these changes Nov 27, 2022

View reviewed changes

HannesWell requested review from laeubi and mickaelistria November 27, 2022 22:53

HannesWell merged commit aa212c4 into eclipse-m2e:master Nov 28, 2022

This was referenced Nov 30, 2022

Cannot invoke "org.apache.maven.artifact.versioning.VersionRange.getRecommendedVersion()" because "vr" is null #1103

Closed

M2E wrongly uses enforcer requireJavaVersion to configure projects #1099

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only evaluate major and minor version of enforcer java rule #981

Only evaluate major and minor version of enforcer java rule #981

kwin commented Oct 11, 2022

github-actions bot commented Oct 11, 2022 •

edited

Loading

HannesWell left a comment

kwin Nov 7, 2022

laeubi Nov 8, 2022

kwin Nov 8, 2022

laeubi Nov 8, 2022

kwin Nov 8, 2022

laeubi Nov 8, 2022

kwin Nov 23, 2022

laeubi Nov 23, 2022

HannesWell Nov 23, 2022

kwin Nov 25, 2022

github-actions bot commented Nov 7, 2022 •

edited

Loading

kwin commented Nov 25, 2022

HannesWell commented Nov 25, 2022

HannesWell left a comment

laeubi commented Nov 28, 2022

HannesWell commented Nov 28, 2022

Only evaluate major and minor version of enforcer java rule #981

Only evaluate major and minor version of enforcer java rule #981

Conversation

kwin commented Oct 11, 2022

github-actions bot commented Oct 11, 2022 • edited Loading

Unit Test Results

HannesWell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 7, 2022 • edited Loading

Test Results

kwin commented Nov 25, 2022

HannesWell commented Nov 25, 2022

HannesWell left a comment

Choose a reason for hiding this comment

laeubi commented Nov 28, 2022

HannesWell commented Nov 28, 2022

github-actions bot commented Oct 11, 2022 •

edited

Loading

github-actions bot commented Nov 7, 2022 •

edited

Loading