RFR: JDK-8157850 Jar tests should pass through VM options

[David Holmes]

On 27/05/2016 2:20 AM, Andrey Nazarov wrote:
> Thanks for feedback guys.
>
> I've updated review http://cr.openjdk.java.net/~anazarov/8157850/webrev.02/

Using test.tool.vm.opts seems reasonable for jar and javac.

> Please sponsor this patch if you are OK.

Sorry not my area.

Thanks,
David

> My use case is to run tests with different -Xms and -Xmx options. Mostly
> due to I need to increase heap size to gather code coverage by jcov.
>
> --Andrey
>
> On 25.05.2016 23:42, Jonathan Gibbons wrote:
>>
>>
>> On 05/25/2016 01:33 PM, David Holmes wrote:
>>> On 26/05/2016 6:04 AM, Jonathan Gibbons wrote:
>>>> Using JAVA_OPTIONS for tools is conceptually wrong.
>>>>
>>>> JAVA_OPTIONS is specifically intended to pass options to VM instances,
>>>> as created by a "java" command.  It is not intended that you should
>>>> prefix the options with -J and use them for arbitrary tools.
>>>
>>> I have to agree. There is no guarantee the options being passed for
>>> the VM under test make any sense for the running of the jar tool in
>>> the jar tests. I think a number of hotspot related test options could
>>> cause problems here.
>>>
>>> Are there specific VM options that people think should be passed
>>> through? The bug report has no detail at all.
>>>
>>> David
>>
>> Generally, I think that blindly passing through all the options
>> regardless is as bad a programming practice as never passing them
>> though.  There are some that make sense to all VMs, like -ea, -esa
>> etc, and maybe system properties, but for those, the VM options
>> mechanism is generally good enough.  (i.e. system properties
>> test.vm.opts, test.tool.vm.opts)
>>
>> From a jtreg point of view, I'd be interested to know uses cases for
>> passing additional more specific options to the VMs used to run tools
>> like jar, jlink, javac, etc
>>
>> -- Jon.
>>
>>
>>
>>>
>>>> The code in the webrev is also perverse for taking the trouble to split
>>>> the string to a stream, collect the results into a list, convert the
>>>> list back into a stream again and use .forEach.
>>>>
>>>> You could do better, and much simpler, with something like
>>>>
>>>>     if (!option.isEmpty()) {
>>>> commands.addAll(Arrays.asList(option.split("\\s+",-1)));
>>>>     }
>>>>
>>>> -- Jon
>>>>
>>>>
>>>> On 05/25/2016 10:48 AM, Martin Buchholz wrote:
>>>>> Hi Andrey,
>>>>>
>>>>> Looking at http://openjdk.java.net/jtreg/vmoptions.html,
>>>>> I see we have both test.vm.opts and test.tool.vm.opts
>>>>> and the latter is supposed to take care of adding "-J".
>>>>>
>>>>> +        VM_OPTIONS.stream().map(opt -> "-J" +
>>>>> opt).forEach(commands::add);
>>>>> +        JAVA_OPTIONS.stream().map(opt -> "-J" +
>>>>> opt).forEach(commands::add);
>>>>>
>>>>> ---
>>>>>
>>>>> Maybe splitting on "\\s+" would be better.
>>>>>
>>>>> ---
>>>>>
>>>>> I think we should have test library methods to return the List<String>
>>>>> for java subprocesses, one that could try hard to get the option
>>>>> tokenization correct.
>>>>>
>>>>> ---
>>>>>
>>>>>
>>>>> On Wed, May 25, 2016 at 9:07 AM, Andrey Nazarov
>>>>> <andrey.x.nazarov at oracle.com> wrote:
>>>>>> Some jar tests start VMs without passing vmoptions from jtreg.
>>>>>>
>>>>>> This fix pass jtreg's vmoptions and javaoptions to processes(java,
>>>>>> jar,
>>>>>> javac) started by tests.
>>>>>>
>>>>>> webrev: http://cr.openjdk.java.net/~anazarov/8157850/webrev.01/
>>>>>>
>>>>>> jbs: https://bugs.openjdk.java.net/browse/JDK-8157850
>>>>>>
>>>>>> --Andrey
>>>>>>
>>>>
>>
>