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

Proper Selection of Execution Environment for Java #1120

Closed
hazendaz opened this issue Dec 4, 2022 · 7 comments · Fixed by #1137
Closed

Proper Selection of Execution Environment for Java #1120

hazendaz opened this issue Dec 4, 2022 · 7 comments · Fixed by #1137
Assignees

Comments

@hazendaz
Copy link

hazendaz commented Dec 4, 2022

This ticket is an effort to bring together many other tickets that have existed since m2e 2.0 release which has negatively impacted java determination within Eclipse. Example simple projects used here to test out the process.

Related Tickets

Versions used

  • Eclipse 2022-09 (4.25.0)
  • m2e 2.1.3.20221203-1403
  • No execution environments directly set before JavaSE-1.8
  • No execution environment directly set for no longer supported versions: 9, 10, 12, 13, 14, 15, 16, 18
  • Not using the beta jdk 19 support on this copy
  • Execution environment JavaSE-1.8 set to jdk 1.8.0.351
  • Execution environment JavaSE-11 set to jdk 11.0.17
  • Execution environment JavaSE-17 set to jdk 17.0.5
  • Installed jdks: 1.8.0.351, 11.0.17, 17.0.5, 19.0.1, 20ea (from end of october)
  • All plugins are up to date in samples used with some minor variation on plugin updates in last 3 months or so.

Test project to be used (small and simple)

Settings involved with relative usage

    <!-- Maven compiler options -->
    <java.version>8</java.version>
    <java.release.version>8</java.release.version>
    <java.test.version>11</java.test.version>
    <java.test.release.version>11</java.test.release.version>

    <!-- source/target are deprecated, remove in future release -->
    <maven.compiler.source>${java.version}</maven.compiler.source>
    <maven.compiler.target>${java.version}</maven.compiler.target>
    <maven.compiler.testSource>${java.test.version}</maven.compiler.testSource>
    <maven.compiler.testTarget>${java.test.version}</maven.compiler.testTarget>

    <maven.compiler.release>${java.release.version}</maven.compiler.release>
    <maven.compiler.testRelease>${java.test.release.version}</maven.compiler.testRelease>

Enforcer plugin is used along with extra enforcer rules to do the following.
- Force byte code compliance based on the 'java.version' argument - That is to say 8 for ehcache-cache sample and 11 for caffeine-cache sample.
- Require java version is set to a range of 11, 17, 18, 19, and 20.  Note the parent pom here was released on Sept 5 and thus 18 was viable then.

A profile is auto activated for m2e when m2e.version is present thus in Eclipse workspace.  This is done to add lifecycle mappings plugins lack or adjustments from the default meta data as well as telling eclipse to use the split 'test' variation of compiler legacy args (so older eclipses and m2e are supported and not sure m2e moved to use release yet).  This logic using the test side split was to overcome fact that older m2e (and possibly latest) did not support it and would not choose the right one in 1.x.  In both examples, this forces to 11.

    <profile>
      <id>eclipse</id>
      <activation>
        <property>
          <name>m2e.version</name>
        </property>
      </activation>

....all the lifecycle stuff followed by compiler work around.

            <!-- 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>

The super pom is used across 20 or so underlying projects in separate repos. Mybatis current policy is that we support java 8 runtimes unless override higher on downstream projects. The default is telling the compiler to be java 8. Its using both the legacy source/target and newer release args split for source vs test. Legacy ones used as some plugins still require it. M2e must be concerned of highest java version to use that will correctly work to build. This means across all projects in mybatis it would be 11 because both tests set to that level and many plugins require that level which enforcer further states. The sample caffine-cache project aligns that directly to 11 only overall.

Test import requirements

  • No prior .settings folder
  • No .project file
  • No .classpath file
  • Import as a maven project using the pom

Current m2e import results

  • Setting JRE System Library to JRE-1.1

m2e should not consider execution environments that have not be setup which would fix the JRE-1.1 issue. If intent is to always use a execution environment, then alert a warning or error on import that no match is set and direct user to set them up first.

m2e should not longer consider these. In fact, Eclipse really should remove them. While I could potentially still see java 1.5 being used in extreme cases, its implausible that others are and really if those old ones are used, I don't think user should be using any modern IDE.

  • CDC-1.0/Foundation-1.0
  • OSGi/Minimum-1.0
  • CDC-1.1/Foundation-1.1
  • JRE-1.1
  • OSGi/Minimum-1.1
  • J2SE-1.2
  • OSGi/Minimum-1.2
  • J2SE-1.3
  • J2SE-1.4
  • J2SE-1.5

In the samples provided, expectation is that the following is final resolution

  • caffeine-cache -> JavaSE-11
  • ehcache-cache -> JavaSE-11

Rules

  • Enforcer plugin is just a hint on what java versions are acceptable to use from a potential list. This addressed the dreaded java class error that is not as nice as telling user to use correct java version. That could be just 1 or more listed versions that are acceptable. The lowest version listed is the version that 'must' be used if the project fails to reach that level with compiler setup.
  • Note that source and test can be split. The higher required java version must be used to set execution environment after reviewing the rules of enforcer if present.
  • Thus, if enforcer says its ok for 11, 17, 18, 19, or 20 in samples provided that is the hint list on what is acceptable. The actual determination then comes after looking at the arguments set on final resolved compiler setup. Those can be split although seen here it was mitigated by accepting long term that m2e didn't handle such a split. But lets say it does now, it needs to determine what is the higher requirement for the build to function between source and test needs. In the examples given, that version is 11 in both cases. With that version, it should then look in that enforcer list if provided and check to see that 11 is there. Lets say 11 is not there, then it should fail with error that enforcer requires version of list X,X,X and build expects X. That way user can fix the issue. Further, once it knows it needs to use 11 for example, that better be set as an execution environment. If it is not, it should error telling user to setup execution environment for 11 so its not guessing as I suspect that is what its doing now since I got JRE-1.1.
@kwin
Copy link
Member

kwin commented Dec 9, 2022

I can reproduce and the issue is https://github.com/mybatis/parent/blob/fcf4376a917a2ccbe72b9dc6de0a6eb99f6c32c5/pom.xml#L612 which sets the version to 11,17,18,19,20 (https://github.com/mybatis/parent/blob/fcf4376a917a2ccbe72b9dc6de0a6eb99f6c32c5/pom.xml#L267). This is currently not properly understood by

private String getMinimumJavaBuildEnvironmentId(String versionSpec) throws InvalidVersionSpecificationException {
. According to https://maven.apache.org/enforcer/enforcer-rules/requireJavaVersion.html this seems to be like an invalid value (as this is clearly not a range) but I have to investigate how the m-enforcer-p treats it internally.

@kwin
Copy link
Member

kwin commented Dec 9, 2022

Actually even m-enforcer-p does not behave like intended, because the full string 11,17,18,19,20 is treated as one recommended version and then compared with the actual version in https://github.com/apache/maven-enforcer/blob/543e46fc654dfc4755bec8f81a0ff96e181cd749/enforcer-rules/src/main/java/org/apache/maven/plugins/enforcer/AbstractVersionEnforcer.java#L133. This accepts any version above 11 (so even potentially version 999). I opened mybatis/parent#401 for this.

This is a totally valid version constraint (although its semantics are different than intended). For details refer to https://issues.apache.org/jira/browse/MRESOLVER-306?focusedCommentId=17645347&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17645347

What is special about this recommended version is that it neither specifies a major nor minor version, therefore the logic added in context of #842 cannot be applied here.

@kwin
Copy link
Member

kwin commented Dec 9, 2022

The attached PR selects JRE environment Java-SE12 for the provided examples, because "11" does not satisfy the requirement (neither with m-enforcer-p), only something like "11.0.1" satisfies the given version constraint but since this cannot be enforced with Java-SE11 the next higher environment is picked.

kwin added a commit to kwin/m2e-core that referenced this issue Dec 9, 2022
HannesWell pushed a commit to kwin/m2e-core that referenced this issue Dec 10, 2022
@hazendaz
Copy link
Author

mybatis-parent is now fixed as follows: [11,12),[17,18),[19,20),[20,21), this will allow any 11, any 17, any 19, any 20 but nothing else now. Confirmed all the rest, it was allowing any 11 or greater before which was not the intent. Further was able to confirm on github actions as we had 18 still setup which was working until the adjustment then failed as expected.

@hazendaz
Copy link
Author

For now won't release the mybatis parent so this can be used more for efforts here. We are releasing snapshots to sonatype so as long as you add this snippet at end to project you can run on the 37-SNAPSHOT so both variations can be tested at the moment. If that is not needed, I can move to release it instead.

  <repositories>
    <repository>
      <id>sonatype-oss-snapshots</id>
      <name>Sonatype OSS Snapshots Repository</name>
      <url>https://oss.sonatype.org/content/repositories/snapshots</url>
    </repository>
  </repositories>

kwin added a commit to kwin/m2e-core that referenced this issue Dec 12, 2022
The value is automatically derived from maven-enforcer rule requireJava

This closes eclipse-m2e#1134
This closes eclipse-m2e#1120
@kwin kwin self-assigned this Jan 5, 2023
HannesWell added a commit to kwin/m2e-core that referenced this issue Feb 28, 2023
The value is automatically derived from maven-enforcer rule requireJava

Closes eclipse-m2e#1134
Closes eclipse-m2e#1099
Closes eclipse-m2e#1120

Co-authored-by: Hannes Wellmann <wellmann.hannes1@gmx.net>
HannesWell added a commit to kwin/m2e-core that referenced this issue Feb 28, 2023
The value is automatically derived from maven-enforcer rule requireJava

Closes eclipse-m2e#1134
Closes eclipse-m2e#1099
Closes eclipse-m2e#1120

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

Please try the latest m2e release to verify that the issue is fixed as expected:

@hazendaz
Copy link
Author

hazendaz commented Mar 2, 2023

@HannesWell Looks like we are now in working order. Tried jdk 17, 11, and 8 and all came up as expected on fresh imports. Thank you! Was getting ready to cut our corporate shared Eclipse and give up on this actually so glad to see it land right as I'm doing that in coming days. Appreciate the hard work and hoping also some of the snappiness I'm seeing sticks ;)

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