-
Notifications
You must be signed in to change notification settings - Fork 91
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
Fixes for duplicate jvm options #1779
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one more comment.
We're leaving alone the fact that I can do a POM property of <liberty.jvm.ab>123</liberty.jvm.ab>
at the same time as -Dliberty.jvm.ab=456
and end up with both "123", "456" entries in the jvm.options.
In other words, unlike other properties in Maven, these two property sets, the sys props and project props, do not get merged before their values are read.
It's been like this for awhile now... but it does strike me as maybe a bit unusual, now that we are looking at it again carefully. And I guess if we're ever going to change this now'd probably be the time. Though on the flip side, one could argue the behavior is already out there and it'd be a migration thing.
And this probably doesn't even matter all that much. I guess I think though that it's a bit more normal or symmetric to merge the property values. What do you think?
liberty-maven-plugin/src/main/java/io/openliberty/tools/maven/server/StartDebugMojoSupport.java
Outdated
Show resolved
Hide resolved
Yes, because jvm options do not have a name->value relationship like every other kind of property/variable that LMP processes. We simply made it fit into that pattern to process them as properties, but we ignore the |
Right, I get the difference between the other config types. But to argue the point some more, consider the case where a parent has a project property The 'abc' value is lost, not included in the gen'd jvm.options, overridden by the child override of this project property. So there still is a name->value relationship (a property) as the |
Ok, so if we stored the props for jvm options in a Map (like the other config that can be specified in props), then we could achieve the overriding that you are talking about. And we would just ignore the I think I would still need the change to avoid duplicate values though. And I would have to maintain the order as well (which is why it is using a List today). |
@scottkurz This is ready for re-review. Note that I had to change the debug option to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There's a lot more logic in the nuance here with the ordering than it seems there might be on first glance. Good to have the test. Hopefully this is all for that one ! Thanks.
Fixes #1778
Additional changes to be included which were discussed with @scottkurz:
jvmOptions
should be ignored.Nothing related to the
jvmOptionsFile
behavior is changing. All the above properties/parameters take precedence over ajvmOptionsFile
or ajvm.options
file in theconfigDirectory
System property | Maven project property | jvmOptions parameter
-Dliberty.jvm.minHeap=-Xms512m
|<liberty.jvm.minHeap>-Xms1024m</liberty.jvm.minHeap>
| NoneNone |
<liberty.jvm.maxHeap>-Xmx2056m</liberty.jvm.maxHeap>
|<param>-Xmx1024m</param>
-Dliberty.jvm.debug="-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=7777"
|<liberty.jvm.debug>-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=7777</liberty.jvm.debug>
|<param>-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=7777</param>
-Dliberty.jvm.myprop1="-Dmy.jvm.prop=abc"
|<liberty.jvm.myprop2>-Dmy.jvm.prop=abc</liberty.jvm.myprop2>
| NoneNone | None. |
<param>-Dmy.jvm.prop2=This is the value</param>
(specified in both a parent pom.xml and the project pom.xml)The generated file should have the following in this order (although order within a section is not guaranteed):
-Xmx2056m
(from Maven project property)-Xms512m
(from System property overriding project property)-Dmy.jvm.prop=abc
(from System property that had dup value of Maven project property)-Xmx1024m
(from jvmOptions parameter)-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=7777
. (from jvmOptions parameter that had dup value of both a System prop and Maven project prop)-Dmy.jvm.prop2=This is the value
(from jvmOptions parameter specified in both parent and project pom.xml - should only appear once)