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

[Ioi Lam]

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.

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
>>>>>
>>>>>
>>>
>