I approve this change.
On Mon, May 30, 2016 at 4:59 PM, David Holmes <david.hol...@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.naza...@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 >>>>>>> >>>>> >>> >> >
