Pushed. Can someone give Andrey some commit bits?
On Tue, May 31, 2016 at 10:23 AM, Martin Buchholz <marti...@google.com> wrote: > 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 >>>>>>>> >>>>>> >>>> >>> >>
