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

[Martin Buchholz]

I approve this change.

On Mon, May 30, 2016 at 4:59 PM, David Holmes <david.holmes at oracle.com> wrote:
> 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
>>>>>>>
>>>>>
>>>
>>
>