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

[Martin Buchholz]

Pushed.

Can someone give Andrey some commit bits?

On Tue, May 31, 2016 at 10:23 AM, Martin Buchholz <martinrb at google.com> wrote:
> 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
>>>>>>>>
>>>>>>
>>>>
>>>
>>