RFR (XS): Run SPECjbb in headless mode in JPRT

[Staffan Larsen]

The changes look good.

I'm in favor of having bug ids in the code. As Mikael says, it makes it much easier to find and understand the background of a fix. If you later change the code so that the fix isn't valid anymore, then the comment should of course be updated.

Thanks,
/Staffan

On 30 maj 2012, at 05:39, Vladimir Kozlov wrote:

> On 5/29/12 8:11 PM, David Holmes wrote:
>> On 30/05/2012 10:48 AM, Mikael Vidstedt wrote:
>>> On 2012-05-29 15:24, Vladimir Kozlov wrote:
>>>> We usually don't add Bug ID into code change. I think the comment
>>>> could be general: "Prevent popups during testing".
>>> 
>>> There are many references to bug IDs in HotSpot and I'd argue that it's
>>> extremely useful to have a link to the bug in the actual source code
>>> since that allows me to more easily find more information about why the
>>> fix was made, along with the potential discussion in the bug tracker.
>> 
>> As this is a specific workaround (that might be removed in the future?) I think tagging it with the bug id is ok.
>> 
>> But, historical baggage notwithstanding, generally we do not want to associate bug ids with code changes. It should be
>> the exception not the norm.
> 
> Totally agree. We have 'hg log' and other tools if you want to see bug ID associated with changes. And I think it is not workaround but a fix which will stay. Why you want to enable popups for batch jobs?
> 
> The problem with bug IDs in a code is the code could be changed later for an other bug so the comment will be not correct unless you start to list all bugs IDs which is not realistic and wrong. Or you will remove bug ID later, then why to have it at all.
> 
> We have some Bug ID and comments which say "the next change is done until such bug is fixed". But even such case should be the exception.
> 
> Thanks,
> Vladimir
> 
>> 
>> Cheers,
>> David
>> -----
>> 
>>> Mentioning the actual bug and the reason for doing the fix in the
>>> comment also serves as a warning for others that may feel tempted to
>>> remove the line going forward.
>>> 
>>> If it's ok with you I'd prefer to keep the comment the way it is?
>>> 
>>> Thanks,
>>> Mikael
>>> 
>>>> 
>>>> Vladimir
>>>> 
>>>> Mikael Vidstedt wrote:
>>>>> 
>>>>> New webrev below. The list of targets now matches the linux x64 list
>>>>> (apart from the platform name that is).
>>>>> 
>>>>> http://cr.openjdk.java.net/~mikael/7155453/webrev.02/
>>>>> 
>>>>> Can I get a couple of Reviews and a sponsor?
>>>>> 
>>>>> Thanks,
>>>>> Mikael
>>>>> 
>>>>> On 2012-05-23 17:27, Vladimir Kozlov wrote:
>>>>>> You mean missing "comma and backslash"?:
>>>>>> 
>>>>>> ${jprt.my.macosx.x64}-{product|fastdebug}-c2-GCOld_ParOldGC, \
>>>>>> 
>>>>>> Vladimir
>>>>>> 
>>>>>> Mikael Vidstedt wrote:
>>>>>>> 
>>>>>>> After addressing the missing backslash it now passes through JPRT
>>>>>>> (on all platforms).
>>>>>>> 
>>>>>>> Cheers,
>>>>>>> Mikael
>>>>>>> 
>>>>>>> On 2012-05-22 17:20, Vladimir Kozlov wrote:
>>>>>>>> Did you run it through JPRT testing?
>>>>>>>> 
>>>>>>>> Vladimir
>>>>>>>> 
>>>>>>>> Mikael Vidstedt wrote:
>>>>>>>>> 
>>>>>>>>> Thanks for the review. I updated based on your feedback.
>>>>>>>>> 
>>>>>>>>> This new webrev also re-enables the JBB testing on OSX:
>>>>>>>>> 
>>>>>>>>> http://cr.openjdk.java.net/~mikael/7155453/webrev.01
>>>>>>>>> 
>>>>>>>>> Cheers,
>>>>>>>>> Mikael
>>>>>>>>> 
>>>>>>>>> On 2012-05-22 13:24, Daniel D. Daugherty wrote:
>>>>>>>>>> On 5/22/12 2:16 PM, Mikael Vidstedt wrote:
>>>>>>>>>>> 
>>>>>>>>>>> All,
>>>>>>>>>>> 
>>>>>>>>>>> I'd like to get a couple of reviews of this change.
>>>>>>>>>>> 
>>>>>>>>>>> We've seen problems with intermittent and hard to debug popups
>>>>>>>>>>> when running SPECjbb on lights out OSX machines. To prevent the
>>>>>>>>>>> popups from blocking the completion of the tests this change
>>>>>>>>>>> makes SPECjbb run in headless mode in JPRT.
>>>>>>>>>>> 
>>>>>>>>>>> http://cr.openjdk.java.net/~mikael/7155453/webrev.00
>>>>>>>>>> 
>>>>>>>>>> Thumbs up.
>>>>>>>>>> 
>>>>>>>>>> make/jprt.properties
>>>>>>>>>> typo: "popup:s" -> "popups"
>>>>>>>>>> 
>>>>>>>>>> The comment kind of implies that this is a MacOS X only change,
>>>>>>>>>> but it isn't. Maybe add one more line:
>>>>>>>>>> 
>>>>>>>>>> # but the work-around is added to all platforms to be consistent
>>>>>>>>>> 
>>>>>>>>>> Dan
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>> 
>>>>>>> 
>>>>> 
>>>