HV-1187 Support JDK9 ea+148 #606
Conversation
gunnarmorling
left a comment
gunnarmorling
left a comment
There was a problem hiding this comment.
@gsmet Looks good overall. Some commenting would be nice so we know (roughly) which each add-opens is needed for so we can hopefully remove them again soon.
On the sharing of locations, it seems not to happen across metadata providers, right? Not a big problem, I'm still wondering though whether we only should switch to accessible members when creating the aggregated model. Then we could do it exactly once. Not sure though whether it's worth the effort. Maybe something for later (also to reduce conflicts with my concurrent work).
README.md
Outdated
There was a problem hiding this comment.
Could you add a comment saying which is needed for what? So we can track it more easily and check whether we can remove any after a component or plug-in update.
cdi/pom.xml
Outdated
There was a problem hiding this comment.
Is it an optimization to avoid creating a stream instance?
There was a problem hiding this comment.
Yes. It's the common case so I thought we might as well be as efficient as possible.
There was a problem hiding this comment.
Do we even need that any longer?
There was a problem hiding this comment.
Shouldn't it be a TypeArgumentConstraintLocation?
There was a problem hiding this comment.
No, it's the one which will be wrapped later. It's strictly equivalent to the existing code.
Switch to gmavenplus and Groovy 2.4.8-SNAPSHOT Switch to patch-gen-maven-plugin 2.0.1.Alpha3-SNAPSHOT
|
Pushed a new version with fixes and more comments. |
Yes, that might be something we want to do in the future but it's orthogonal to this patch. |
| <properties> | ||
| <surefire.argLine.extension>--add-modules ${jigsaw.modules}</surefire.argLine.extension> | ||
| <maven-surefire-plugin.argLine.add-modules>--add-modules=${maven-surefire-plugin.jigsaw.modules}</maven-surefire-plugin.argLine.add-modules> | ||
| <arquillian.javaVmArguments.add-opens> |
There was a problem hiding this comment.
Wow, that's a whole lot. They are all required by WildFly?
There was a problem hiding this comment.
I took them from the WF patch referenced by Tomaz. I think it's better to be in line with their expectations. I didn't play hide and seek for this one.
|
+1. Maybe only add a comment on where it's from.
|
|
@gunnarmorling pushed a new version:
I think it's ready to go now. I'll work on the backports once this is in. |
|
Looks reat now! Merged, thanks, @gsmet! |
2 participants
Waiting for jbossas/patch-gen#17 to be integrated and a new alpha released.
Uses Groovy 2.4.8-SNAPSHOT as it has important fixes regarding JDK9 ea+148 support.