RFR(S) 8193664 [10] - AppCDS tests should use -XX:+UnlockCommercialFeatures when running with commercial JDK

[David Holmes]

On 12/01/2018 10:35 AM, Ioi Lam wrote:
> On 1/10/18 10:10 PM, Ioi Lam wrote:
>> On 1/10/18 10:04 PM, David Holmes wrote:
>>> Hi Ioi,
>>>
>>> On 11/01/2018 2:42 PM, Ioi Lam wrote:
>>>> Hi David,
>>>>
>>>> Thanks for the review. I've removed the duplicated code and added a 
>>>> new method TestCommon.makeCommandLineForAppCDS. The code looks like 
>>>> this now:
>>>>
>>>> !  ProcessBuilder dumpPb = ProcessTools.createJavaProcessBuilder(true,
>>>> !    TestCommon.makeCommandLineForAppCDS(
>>>>         "-XX:+UseAppCDS",
>>>>         "-XX:+UseCompressedOops",
>>>>         "-XX:+UseG1GC",
>>>>         "-cp", appJar,
>>>>         "-XX:SharedArchiveConfigFile=" + sharedArchiveConfigFile,
>>>>         "-XX:SharedArchiveFile=./SharedStringsBasic.jsa",
>>>>         "-Xshare:dump",
>>>> !      "-Xlog:cds,cds+hashtables"));
>>>
>>> Looks good! Thanks for that.
>>>
>>> One comment:
>>>
>>>  public static String[] makeCommandLineForAppCDS(String... args) 
>>> throws Exception {
>>>
>>> why the throws clause?
>>>
>> Oops, that was a mistake. I'll remove it.
>>
> 
> Oh, I checked again, and the throws clause is necessary because this 
> method calls BuildHelper.isCommercialBuild(), which throws Exception. I 
> think the throws clause in the latter method is really not necessary, 
> but that's outside of the scope of this changeset.

It propagates up from:

  properties.load(new FileReader(getReleaseFile()));

so seems unavoidable.

Cheers,
David

> So after all, I will keep the changes the same as the last posted 
> version, and push that today.
> 
> Thanks
> - Ioi
>> Thanks
>> - Ioi
>>> Thanks,
>>> David
>>>
>>>> Updated webrev is at
>>>>
>>>> http://cr.openjdk.java.net/~iklam/jdk10/8193664-commercial-appcds-testbug.v02/ 
>>>>
>>>>
>>>> I also updated the copyright years as mentioned by Jiangli.
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>>
>>>> On 1/10/18 2:31 PM, David Holmes wrote:
>>>>> Hi Ioi,
>>>>>
>>>>> I have two small reservations ...
>>>>>
>>>>> 1. The duplicate definitions of commercial()
>>>>> 2. The use of -showversion in the openJDK case. Might not this 
>>>>> interfere with output parsing?
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>> On 11/01/2018 5:47 AM, Ioi Lam wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review this small change in the AppCDS tests
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8193664
>>>>>> http://cr.openjdk.java.net/~iklam/jdk10/8193664-commercial-appcds-testbug.v01/ 
>>>>>>
>>>>>>
>>>>>> Summary:
>>>>>>
>>>>>>      * This patch changes tests ONLY. There's no code change.
>>>>>>      * The change affects ONLY the commercial JDK built by Oracle.
>>>>>>      * It does not affect the OpenJDK
>>>>>>
>>>>>>      * Oracle commercial JDK 10 still requires 
>>>>>> -XX:+UnlockCommercialFeatures
>>>>>>        to be specified when using AppCDS tests. This requirement 
>>>>>> will be
>>>>>>        removed soon in the future, but meanwhile, we need to add a 
>>>>>> few
>>>>>>        -XX:+UnlockCommercialFeatures flags in the test cases to 
>>>>>> make them pass.
>>>>>>
>>>>>> Tested under both Oracle commercial JDK and OpenJDK builds. All 
>>>>>> affected
>>>>>> tests passed in both configurations.
>>>>>>
>>>>>> Thanks
>>>>>> - Ioi
>>>>>>
>>>>>>
>>>>
>>
>