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

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

Closed
CsCherrYY opened this issue Jul 6, 2022 · 50 comments · Fixed by #981
Closed

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

CsCherrYY opened this issue Jul 6, 2022 · 50 comments · Fixed by #981

Comments

@CsCherrYY
Copy link
Contributor

steps to reproduce:

  1. given sample project: https://github.com/mybatis/mybatis-3.git
  2. open it in VS Code, with redhat.java extension 1.8.0 installed (which integrates m2e 2.0.0 snapshot).
  3. after project imported, VS Code reports an error in Item record under package org.apache.ibatis.submitted.record_type.

java.lang.Record cannot be resolved to a type.

as we know, record should be available in Java 16 but not in Java 8.

  1. check the generated .classpath file, the JRE_CONTAINER is set to JavaSE-1.8 VM.

classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-1.8"

  1. check the generated .settings/org.eclipse.jdt.prefs file, the target platform is set to Java 16 (which would be the expect source level since it's defined in https://github.com/mybatis/mybatis-3/blob/9da3467e96a03e8bd6e040139581a22ce5297c29/pom.xml#L449-L459)

org.eclipse.jdt.core.compiler.codegen.targetPlatform=16
org.eclipse.jdt.core.compiler.compliance=16
org.eclipse.jdt.core.compiler.source=16

if we rollback to redhat.java 1.6.0, which uses m2e 1.18.2, this issue disappears. The JRE_CONTAINER in .classpath file is JavaSE-16, and the value of pref org.eclipse.jdt.core.compiler.codegen.targetPlatform is also 16.

I think this issue might related to #597 which introduces a new limitation from maven-enforcer-plugin. But I'm not sure it's caused by the plugin or m2e itself. When debugging, the two environments seem to mismatch:
image

cc: @fbricon @rgrunber

@mickaelistria
Copy link
Contributor

Can you please provide steps to reproduce with plain Eclipse m2e? Or even better, as a unit test for org.eclipse.m2e.jdt.tests ?

@rgrunber
Copy link
Contributor

rgrunber commented Jul 6, 2022

I am able to reproduce with :

Version: 2022-06 (4.25)
Build id: I20220704-0600
M2E - Maven Integration for Eclipse 2.0.0.20220705-1608 org.eclipse.m2e.feature.feature.group Eclipse.org - m2e

This does not occur with the 1.x m2e :

M2E - Maven Integration for Eclipse (includes Incubating components) 1.20.1.20220227-1319 org.eclipse.m2e.feature.feature.group Eclipse.org - m2e

I just had to add a Java 1.8 JDK to "Installed JREs" so that it may be detected (if needed). Otherwise, you'll see the runtime as JavaSE-1.8 (with an incompatible JDK underlying it which is a warning by default)

The pom file does have a profile :

    <profile>
      <id>16</id>
      <activation>
        <jdk>[16,)</jdk>
      </activation>
      <properties>
        <maven.compiler.testTarget>16</maven.compiler.testTarget>
        <maven.compiler.testSource>16</maven.compiler.testSource>
        <excludedGroups>TestcontainersTests,RequireIllegalAccess</excludedGroups>
      </properties>
    </profile>

Is this just a regression in detecting the Java runtime from the profile ?

@mickaelistria
Copy link
Contributor

@rgrunber do you think you could turn this example into a test case for org.eclipse.m2e.jdt.tests?
@kwin any idea which part of the code too look at, that could be causing this inconsistency?

@kwin
Copy link
Member

kwin commented Jul 6, 2022

There are two different potential environment ids relevant:

addJREClasspathContainer(classpath, buildEnvironmentId != null ? buildEnvironmentId : executionEnvironmentId);
. The getMinimumJavaBuildEnvironmentId() is used preferably and determined from the minimum Java enforced through m-enforcer-p while getExecutionEnvironmentId() is driven from maven-compiler-plugin settings (logic introduced in #590).

In this case the enforced Java version seems to be 8 (set via https://repo1.maven.org/maven2/org/mybatis/mybatis-parent/34/mybatis-parent-34.pom). The profile with id 16 doesn't set a value for property maven.compiler.source which is used in the maven-enforcer-plugin.

Not sure if the logic should be improved that always the higher execution id is being taken instead of the one from the m-enforcer-p first....

kwin added a commit to kwin/m2e-core that referenced this issue Jul 6, 2022
m-compile-p and m-enforcer-p parametrisation

This closes eclipse-m2e#842
@mickaelistria
Copy link
Contributor

getExecutionEnvironmentId() is driven from maven-compiler-plugin settings

And why does this return 16 here?

@kwin
Copy link
Member

kwin commented Jul 7, 2022

And why does this return 16 here?

Because the profile with id 16 sets the configuration for the test compile execution (

protected List<MojoExecution> getCompilerMojoExecutions(ProjectConfigurationRequest request, IProgressMonitor monitor)
) accordingly.

@mickaelistria
Copy link
Contributor

ah ok, I didn't notice this profile had an auto-activation set. But the properties it sets are for tests, so shouldn't those be ignored ?

@kwin
Copy link
Member

kwin commented Jul 7, 2022

All mojo executions are considered at

for(MojoExecution execution : getCompilerMojoExecutions(request, monitor)) {
and the last one actually wins (i.e. determines the compiler options).

@mickaelistria
Copy link
Contributor

All mojo executions are considered at

OK, so that's probably the root cause of the bug. The test executions should be skipped.

@kwin
Copy link
Member

kwin commented Jul 7, 2022

Not sure, if maven-enforcer enforces a lower version, but you have a profile (conditionally activated) setting the target of the compiler (not only for test) to a higher version you also run into this issue. So in general the JRE container should be the highest one from the different parameterisations.

@mickaelistria
Copy link
Contributor

Not sure, if maven-enforcer enforces a lower version, but you have a profile (conditionally activated) setting the target of the compiler (not only for test) to a higher version you also run into this issue

That's not really the case here as only test config is overridden and there is no reason that the test config is used to derive main project settings. Moreover a pom setup that mixes different version in enforcer and main compilation would be inconsistent even from Maven POV, so no need to worry further about it in m2e.
Here the test compilation should be ignored, or only the default compilation considered. Other compilations are supplemental and manage things on a grain the doesn't map well in Eclipse m2e anyway.

@CsCherrYY
Copy link
Contributor Author

Here the test compilation should be ignored, or only the default compilation considered. Other compilations are supplemental and manage things on a grain the doesn't map well in Eclipse m2e anyway.

@mickaelistria In this way, IIUC both the project's compliance and the JRE container of that sample project would be set to 1.8 since only the default compilation considered, right? And can we do something with the tests in source level 16?

@mickaelistria
Copy link
Contributor

IIUC both the project's compliance and the JRE container of that sample project would be set to 1.8

Yes.

And can we do something with the tests in source level 16?

No. JDT cannot be configured to compile the sources from a given project with different compiler options (eg source level). Changing that would require a fair amount of work in JDT, to move the various options from project-scope only to source-folder scope. Then, when JDT can do it, m2e can leverage it.

@CsCherrYY
Copy link
Contributor Author

Yeah, and I'm thinking since JDT use the same compiler option for a project, why not use the highest version between them, as @kwin 's new proposal?

I can imagine that if we set the source compiler level of the project to 1.8, users will encounter errors about record in tests. While if we use the higher of them (16), it can solve this issue and no further compiler error will appear (since language level 16 is compatible with level 8)

@laeubi
Copy link
Member

laeubi commented Jul 11, 2022

No. JDT cannot be configured to compile the sources from a given project with different compiler options (eg source level). Changing that would require a fair amount of work in JDT, to move the various options from project-scope only to source-folder scope. Then, when JDT can do it, m2e can leverage it.

I don't think that it needs to be that complex, JDT already distinguish between "main" and "test" sources and even classpath entries and seem to compile them differently, so it seems possible to even use a different compiler level then for test sources.

@kwin
Copy link
Member

kwin commented Jul 11, 2022

so it seems possible to even use a different compiler level then for test sources.

At least via UI that option is not exposed in Eclipse and having some options for compilation which are not exposed in UI calls for trouble as there is too much magic in it then.

@laeubi
Copy link
Member

laeubi commented Jul 11, 2022

At least via UI that option is not exposed in Eclipse

I don't think it is currently possible, just not that we need a setting per source folder as indicated by @mickaelistria

having some options for compilation which are not exposed in UI calls for trouble as there is too much magic in it then.

Compiling code with higher JVM level could also be a source of problems as it might "leak" into your Java 1.8 code.

@CsCherrYY
Copy link
Contributor Author

JDT already distinguish between "main" and "test" sources and even classpath entries and seem to compile them differently

AFAIK JDT indeed distinguish these two sources but can't use different options to compile them, and only compiler prefs such as "org.eclipse.jdt.core.compiler.compliance" can be found. @laeubi could you please explain a little bit?

@mickaelistria what do you think of this?

@laeubi
Copy link
Member

laeubi commented Jul 11, 2022

JDT needs to be enhanced to have e.g. org.eclipse.jdt.core.compiler.test.compliance ...

@MrThaler
Copy link

MrThaler commented Sep 23, 2022

Moreover a pom setup that mixes different version in enforcer and main compilation would be inconsistent even from Maven POV, so no need to worry further about it in m2e.

The purpose of the enforcer rule is to set a range of acceptable versions, e.g. "Java 8 and above" or "Any Java version between 7 and 11". If that condition is not met, the enforcer rule does not adjust compilation target - it simply aborts the build process.
So it is a perfectly valid setup to enforce a Java version of Java 8 and above, but actually compile with and for Java 11. I would argue this is even the purpose of that particular enforcer rule.

As it is right now, the Build Process would state "I'm compiling with Java 11 for Java 11. I'm also verifying that both of these versions are newer than Java 8" and m2e would go "Clearly we should use Java 8 to run this then" - in essence turning a rule that says "At least one version in range must be able to compile and run the code" into "The lowest version in range must be able to compile and run the code" which is not a bug in the pom.xml, but m2e.

@kwin
Copy link
Member

kwin commented Sep 23, 2022

So it is a perfectly valid setup to enforce a Java version of Java 8 and above, but actually compile with and for Java 11.

This is just not possible, as the compiler uses the same JDK as the rest of the build by default. And Java 8 cannot compile for Java 11.

@hazendaz
Copy link

hazendaz commented Sep 25, 2022

I'm the maintainer of the mybatis projects maven builds. The parent pom is set to detect m2e.version property. Does that even exist still? This worked prior to 2.x m2e jump.

Via this profile (from https://github.com/mybatis/parent/blob/master/pom.xml)

    <profile>
      <id>eclipse</id>
      <activation>
        <property>
          <name>m2e.version</name>
        </property>
      </activation>
            <!-- Force Eclipse to use test side in order to ensure no compilation errors (generally these are same and make no difference) -->
            <plugin>
              <groupId>org.apache.maven.plugins</groupId>
              <artifactId>maven-compiler-plugin</artifactId>
              <configuration>
                <source>${maven.compiler.testSource}</source>
                <target>${maven.compiler.testTarget}</target>
              </configuration>
            </plugin>

So the issue here should be setting the entire thing to jdk 16 as required for mybatis-3 to compile tests at least and known that it all must go there even though bulk is jdk 8 based code. We have also moved off using jdk 8 at all for builds now. So currently, a fresh import of mybatis-3 shows up with JavaSE-12 being selected. I'm unclear where 12 is even coming from. The required java version in parent on enforcer plugin is an allowable range 11, 17, 18, 19, 20 in our latest.

@laeubi
Copy link
Member

laeubi commented Sep 26, 2022

As far as I know nothing has changed here. The JVM itself are configured under Window > Preferences > Java > Installed JREs there you can see what JRE maps what java version.

What m2e actually has derived, should be stored in <your project>/.settings/org.eclipse.jdt.core.prefs > org.eclipse.jdt.core.compiler.compliance=<derived java version>

@laeubi
Copy link
Member

laeubi commented Sep 26, 2022

This is just not possible, as the compiler uses the same JDK as the rest of the build by default. And Java 8 cannot compile for Java 11.

Tycho can compile with any java version, also plain maven can do this, using configured toolchains.

@hazendaz
Copy link

ok the file itself shows 16. odd though its not selecting that in the ide.

@MrThaler
Copy link

MrThaler commented Sep 27, 2022

So it is a perfectly valid setup to enforce a Java version of Java 8 and above, but actually compile with and for Java 11.

This is just not possible, as the compiler uses the same JDK as the rest of the build by default. And Java 8 cannot compile for Java 11.

You are misunderstanding me. I'm not saying to use Java 8 to compile for Java 11. I say it is possible that you enforce a minimum of Java 8, but are actually using Java 11.
If I have a sign stating "You must be 5' to enter" and I'm 6' nobody expects me to chop off my legs to get down to size. It's a minimum (or possibly a range), as clearly stated in the documentation..
If I have an enforcer rule stating "All public methods must have a Javadoc" I am still free to write Javadoc for my private methods.
It's a valid setup for a company to utilize an enforcer rule in a parent pom to force all of their developers to use a minimum of Java 8, but allows for them to use Java 11 in their projects if they so desire.

Again, it is not the job of the enforcer rule to set the used Java version. It's the job of the enforcer rule to validate that the used Java version follows the given guidelines. So m2e ignoring the actually used version in the build and instead utilizing the enforced minimum version is simply wrong behavior, ignoring something explicitly set for a minimum it basically guesses.
This leads to the result that the pom is valid and mvn install works perfectly fine (using Java 11 as set in the pom) while at the same time Eclipse claims the code to be impossible to compile because it uses methods not yet present in Java 8 (because it incorrectly deduces the code is written for Java 8).

@kwin
Copy link
Member

kwin commented Oct 11, 2022

The version being given in rule requireJavaVersion is always a range: https://maven.apache.org/enforcer/enforcer-rules/versionRanges.html.
That means that <version>17.0.4</version> is equivalent to <version>[17.0.4,)</version>, i.e. everything greater or equal than 17.0.4 works. However Eclipse only knows major environment version ids, therefore it sets the minimum version to 18 in that case.

The exact source code is in

private String getMinimumJavaBuildEnvironmentId(String versionSpec) throws InvalidVersionSpecificationException {
VersionRange vr = VersionRange.createFromVersionSpec(versionSpec);
ArtifactVersion recommendedVersion = vr.getRecommendedVersion();
// find lowest matching environment id
for(Entry<String, String> entry : ENVIRONMENTS.entrySet()) {
ArtifactVersion environmentVersion = new DefaultArtifactVersion(entry.getKey());
boolean foundMatchingVersion = false;
if(recommendedVersion == null) {
foundMatchingVersion = vr.containsVersion(environmentVersion);
} else {
// only singular versions ever have a recommendedVersion
int compareTo = recommendedVersion.compareTo(environmentVersion);
foundMatchingVersion = compareTo <= 0;
}
if(foundMatchingVersion) {
return entry.getValue();
}
}
return null;
}
.

Feel free to suggest how to better support this edge case.

@famod
Copy link

famod commented Oct 11, 2022

@kwin thanks for your reply.

Setting [17.0.4] does avoid this problem, so thanks for pointing that out (in fact, I think we had this range set at some point but it might got lost during a refactoring).

However Eclipse only knows major environment version ids, therefore it sets the minimum version to 18 in that case.

Leaving that version range aspect aside for a moment, my major gripe is that m2e shouldn't fall back to a JVM that's not even installed.

@kwin
Copy link
Member

kwin commented Oct 11, 2022

Leaving that version range aspect aside for a moment, my major gripe is that m2e shouldn't fall back to a JVM that's not even installed.

What should happen if Eclipse doesn't find a compatible JRE for the identified minimum execution environment from your PoV?

@kwin
Copy link
Member

kwin commented Oct 11, 2022

kwin added a commit to kwin/m2e-core that referenced this issue Oct 11, 2022
Fix evaluation of expressions in m-enforcer-p parameter
Only select ExecutionEnvironments with at least one compatible VM
installed.
This closes eclipse-m2e#842
@kwin
Copy link
Member

kwin commented Oct 11, 2022

@famod Please try out #981

kwin added a commit to kwin/m2e-core that referenced this issue Oct 11, 2022
Fix evaluation of expressions in m-enforcer-p parameter
Only select ExecutionEnvironments with at least one compatible VM
installed.
This closes eclipse-m2e#842
kwin added a commit to kwin/m2e-core that referenced this issue Nov 7, 2022
Fix evaluation of expressions in m-enforcer-p parameter
Only select ExecutionEnvironments with at least one compatible VM
installed.
This closes eclipse-m2e#842
kwin added a commit to kwin/m2e-core that referenced this issue Nov 25, 2022
Fix evaluation of expressions in m-enforcer-p parameter
Only select ExecutionEnvironments with at least one compatible VM
installed.
This closes eclipse-m2e#842
kwin added a commit to kwin/m2e-core that referenced this issue Nov 27, 2022
Fix evaluation of expressions in m-enforcer-p parameter
Only select ExecutionEnvironments with at least one compatible VM
installed.
This closes eclipse-m2e#842
kwin added a commit to kwin/m2e-core that referenced this issue Nov 27, 2022
Fix evaluation of expressions in m-enforcer-p parameter
Only select ExecutionEnvironments with at least one compatible VM
installed.
This closes eclipse-m2e#842
kwin added a commit to kwin/m2e-core that referenced this issue Nov 27, 2022
Fix evaluation of expressions in m-enforcer-p parameter
Only select ExecutionEnvironments with at least one compatible VM
installed.
This closes eclipse-m2e#842
HannesWell pushed a commit to kwin/m2e-core that referenced this issue Nov 27, 2022
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 pushed a commit that referenced this issue Nov 28, 2022
Fix evaluation of expressions in m-enforcer-p parameter
Only select ExecutionEnvironments with at least one compatible VM
installed.
This closes #842

Also-by: Hannes Wellmann <wellmann.hannes1@gmx.net>
@HannesWell
Copy link
Contributor

Is this issue completely fixed with #981?

@kwin
Copy link
Member

kwin commented Nov 29, 2022

The error reports are all a bit fuzzy here, so the only thing which has actually changed with #981 is that only Java versions which have a compliant installed JRE are selected and that minor versions are disregarded (because Eclipse only distinguishes OSGi execution environments, i.e. major Java versions)

@hazendaz
Copy link

hazendaz commented Dec 3, 2022

@HannesWell Issue is not resolved. With latest m2e 2.1.3, its now importing fresh project noted here as JRE-1.1 and then setting that to jdk 17. JRE-1.1 is not setup for any execution environment and java version was set to 11. It really should not be using enforcer plugin to guess its way to glory. The compiler args already told it what version. This is a large problem when super poms support wide ranges.

@hazendaz
Copy link

hazendaz commented Dec 3, 2022

note: 17 picked above as its the default in my Eclipse. Other concerns still stand, it picks wrong execution environment.

@kwin
Copy link
Member

kwin commented Dec 3, 2022

A lot of different issues have been reported in this ticket. If still a JRE is picked which is not the enforced Java version or the target version then I would suggest to create a new ticket, with detailed information/example pom and expected JRE version and actual JRE version.

@HannesWell
Copy link
Contributor

@HannesWell Issue is not resolved. With latest m2e 2.1.3, its now importing fresh project noted here as JRE-1.1 and then setting that to jdk 17. JRE-1.1 is not setup for any execution environment and java version was set to 11. It really should not be using enforcer plugin to guess its way to glory. The compiler args already told it what version. This is a large problem when super poms support wide ranges.

OK. Could it be that you have the same issue like in #1099? I'm about to look into that.

In general, from my understanding (also influenced by this discussion) of how to use the enforcer-plugin and the source/target/release properties of the maven-compiler-plugin I think M2E should do the following:

  • Configure the JDT ...compiler.source/...compiler.codegen.targetPlatform/...compiler.release preferences according to the corresponding configuration of the maven-compiler-plugin.
  • In general set the JRE_CONTAINER in the project's .classpath to the value of the source/target/release properties of the `maven-compiler-plugin
    • in case of different versions, release wins, and if only source and target are specified the higher of both is used?
    • If a enforcer-plugin execution with requireJavaVersion rule is configured and that rule requires a higher Java version than specified by the compiler-plugin, then the lowest possible java (major) version that satisfies the rule's version range should be set as JRE_CONTAINER.
      Personally I never really used the enforcer plugin yet, but from my understanding it is used to enforce the usage of a JDK with a higher version than the targeted Java version when building the sources. Since one cannot target more recent java versions than the JDK used to build is of, the compiler-plugin settings are the minimum. And reasonably the enforcer-plugin can only require the usage of more recent JDKs (for whatever reason). Therefore I think the suggested approach would best mimic the behavior of the build.

Does anybody disagree with that analysis?
If this is the desired behavior the second step is to make sure M2E acts accordingly.

@Bananeweizen maybe you are interested in this discussion as well.

@hazendaz
Copy link

hazendaz commented Dec 4, 2022 via email

@hazendaz
Copy link

hazendaz commented Dec 4, 2022 via email

@laeubi
Copy link
Member

laeubi commented Dec 4, 2022

Does anybody disagree with that analysis?

I think there are actually two levels:

  1. Compile time (and that's what JRE_CONTAINER maps to and I don't see a reason why enforcer should have any influences here...)
  2. Runtime (e.g. running the tests from within eclipse, running a main-class, ...) where enforcer becomes relevant.

@hazendaz
Copy link

hazendaz commented Dec 4, 2022 via email

@hazendaz
Copy link

hazendaz commented Dec 4, 2022

Opened #1120 that should help this entirely and has sample projects which the most broken up configuration between src/test and legacy vs modern setup and small project samples on live projects that have been configured as such for a long time and should be ideal in helping understand all this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants