Fixes for duplicate jvm options

[cherylking]

Fixes #1778

Additional changes to be included which were discussed with @scottkurz:

1. A system property for a jvm option with the same name should override a Maven project property.
2. A duplicate entry for jvmOptions should be ignored.
3. The order of the values in the generated file should be:

• Maven project properties
• System properties
• jvmOptions

Nothing related to the jvmOptionsFile behavior is changing. All the above properties/parameters take precedence over a jvmOptionsFile or a jvm.options file in the configDirectory

System property | Maven project property | jvmOptions parameter

-Dliberty.jvm.minHeap=-Xms512m | <liberty.jvm.minHeap>-Xms1024m</liberty.jvm.minHeap> | None

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

None | 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)

[scottkurz]

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?

[cherylking]

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.

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 ab portion of that property name in your example. We make reference to the fact we don't use the var name here, but could be more explicit to say it is not used in the merging - only the values are merged.

[scottkurz]

Yes, because jvm options do not have a name->value relationship like every other kind of property/variable that LMP processes.

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 <liberty.jvm.xyz>abc</liberty.jvm.xyz> and a child inheriting from this parent has a project property: <liberty.jvm.xyz>123</liberty.jvm.xyz>.

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 liberty.*.* properties are parsed...it's just there's not a name->value in the ultimate output we produce, within the jvm.options, like there is for the other config: server.xml vars, env vars, etc. And so it'd be more typical, in Maven, when a project property and system property can both be used to supply a single name->value, to have the sysprop override.

[cherylking]

And so it'd be more typical, in Maven, when a project property and system property can both be used to supply a single name->value, to have the sysprop override.

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 key in the Map when merging with the jvmOptions which have no name/key? Would that get the behavior you are thinking we should have?

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).

[cherylking]

@scottkurz This is ready for re-review. Note that I had to change the debug option to use ; instead of , because I could not figure out the syntax to prevent Maven from parsing those commas in the property value when specified from the command line (via invoker.properties).

[scottkurz]

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.