[ELY-2514] Update minimum JDK for all modules to 11 #1880
Conversation
|
|
||
| /** | ||
| * JDK-specific classes which are replaced for different JDK major versions. This class is for JDK 8. | ||
| * JDK-specific classes which are replaced for different JDK major versions. This class is for JDK 9. | ||
| * @author <a href="mailto:[email protected]">Justin Cook</a> | ||
| */ | ||
| final class JDKSpecific { |
There was a problem hiding this comment.
@cam-rod @fjuma do we have plans for the JDKSpecific classes? Can we refactore them out of the codebase? Since they were introduced to resolve some inconsistencies between Java 8 and Java 9 IIUC
There was a problem hiding this comment.
I don't think there's a specific requirement for those classes, so likely yes
There was a problem hiding this comment.
Should I go ahead with refactoring out these classes?
tests/base/pom.xml
Outdated
| <!-- [WFCORE-1431] remove SASL workaround --> | ||
| <modular.jdk.args>--add-modules java.sql --illegal-access=permit</modular.jdk.args> | ||
| <!-- use version of jboss-logging that works much better with JDK9 --> | ||
| <modular.jdk.props>-Djdk.attach.allowAttachSelf=true</modular.jdk.props> |
There was a problem hiding this comment.
@cam-rod pls this does not relate to this PR, but do youhappen to know why these options are needed? and if they are required in tests/base module since they are in parent pom.xml?
I am trying to understand how the comments above these options relate to the options. It is not clear to me why java.sql module relate to some SASL workaround and whether we should configure the allowAttachSelf as a global option if it relates only to the jboss-logging version
There was a problem hiding this comment.
I'm not really sure what these are doing either. Seems to date back to this point in #494, as part of a larger set of args. That larger block showed up in a few spots (example), but they were pinned to JDK 9 and failed on 11, so I don't think it's relevant anymore.
I'll try removing these blocks entirely and see if it changes anything.
There was a problem hiding this comment.
Finished running, removing all references to <modular.jdk.args> and <modular.jdk.props> passed all tests.
Edit: removed the lines for now, if it doesn't make sense then I can add it back
| <!-- use version of jboss-logging that works much better with JDK9 --> | ||
| <modular.jdk.props>-Djdk.attach.allowAttachSelf=true</modular.jdk.props> | ||
| <!-- 2.20.x doesn't start on JDK10--> | ||
| <version.surefire.plugin>2.19.1</version.surefire.plugin> |
There was a problem hiding this comment.
@cam-rod I see that we re-added these options to the poms where they were removed (tests/base/pom.xml and pom.xml). Should we then re-add them here under the <properties> as well?
There was a problem hiding this comment.
Leaning to removing the modular.jdk.props argument entirely based on the other comments I added. The surefire version I think is set by the jboss-parent module by default, and shouldn't be an issue since JDK 11 is the minimum.
|
Superseded by #2109 |
https://issues.redhat.com/browse/ELY-2514
Split into two commits:
This is a rather aggressive removal of configurations (ex. the java8 and java9 test profiles are removed entirely, without replacement), so sections can be readded if needed.