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

[Andrey Nazarov]

Thanks for feedback guys.

I've updated review http://cr.openjdk.java.net/~anazarov/8157850/webrev.02/

Please sponsor this patch if you are OK.

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