RFR: CODETOOLS-7902453: Jtreg JDK.getJDKVersion failed to load GetSystemProperty.class when debugging

[Jonathan Gibbons]

Patrick,

The basic patch looks OK.

I'm seeing some probably-unrelated issues running the jtreg tests, and 
so I'll push your patch when those issues are sorted out.

One the subject of tests, it is strongly desired that all non-trivial 
patches should have a corresponding test. This is not just good 
practice, it should also be seen as a defense mechanism against the 
patch failing in the future. In general, it is not enough to just run 
jtreg once or twise with a patch, and eyeball the results in some 
private configuration. In this case, a reasonable test would be to run 
jtreg with the classes (instead of the jar file) on the classpath.

-- Jon


On 05/05/2019 10:45 PM, Patrick Zhang OS wrote:
> Hi Reviewers,
>
> Could you please help review it? Thanks.
> JBS: https://bugs.openjdk.java.net/browse/CODETOOLS-7902453
> Webrev: http://cr.openjdk.java.net/~qpzhang/codetools-7902453/webrev.03
>
> Regards
> Patrick
>
> -----Original Message-----
> From: code-tools-dev <code-tools-dev-bounces at openjdk.java.net> On Behalf Of Patrick Zhang OS
> Sent: Thursday, April 25, 2019 10:29 AM
> To: Jonathan Gibbons <jonathan.gibbons at oracle.com>; code-tools-dev at openjdk.java.net
> Subject: RE: RFR: CODETOOLS-7902453: Jtreg JDK.getJDKVersion failed to load GetSystemProperty.class when debugging
>
>> That would be inappropriate, and against long-standing jtreg policy of being susceptible to the user's environment.
> Fine, I was thinking the change at least would not make things worse, while it sounds CLASSPATH got intentionally cleared here, and cleanly written with the path of jtreg.jar and javatest.jar.
> Dropped that part, please review this update: http://cr.openjdk.java.net/~qpzhang/codetools-7902453/webrev.03, thanks.
>
> Regards
> Patrick
>
> -----Original Message-----
> From: Jonathan Gibbons <jonathan.gibbons at oracle.com>
> Sent: Thursday, April 25, 2019 1:54 AM
> To: Patrick Zhang OS <patrick at os.amperecomputing.com>; code-tools-dev at openjdk.java.net
> Subject: Re: RFR: CODETOOLS-7902453: Jtreg JDK.getJDKVersion failed to load GetSystemProperty.class when debugging
>
>> I would like to keep the fix for CLASSPATH, which should not be overwritten regardless. Maybe there were predefined values inside from users.
> That would be inappropriate, and against long-standing jtreg policy of being susceptible to the user's environment.
>
> -- Jon
>
>
> On 04/24/2019 12:46 AM, Patrick Zhang OS wrote:
>> Hi Jon,
>> Thanks for your review.
>> Please see my updates: http://cr.openjdk.java.net/~qpzhang/codetools-7902453/webrev.02. Instead of modifying sysPropClassPath directly, I changed JarFinder.getPath() a little bit: if the URI is not JAR, try the root path of class files.
>> I would like to keep the fix for CLASSPATH, which should not be overwritten regardless. Maybe there were predefined values inside from users.
>>
>> Regards
>> Patrick
>>
>> -----Original Message-----
>> From: Jonathan Gibbons <jonathan.gibbons at oracle.com>
>> Sent: Tuesday, April 23, 2019 10:50 PM
>> To: Patrick Zhang OS <patrick at os.amperecomputing.com>;
>> code-tools-dev at openjdk.java.net
>> Subject: Re: RFR: CODETOOLS-7902453: Jtreg JDK.getJDKVersion failed to
>> load GetSystemProperty.class when debugging
>>
>> Since sysPropClassPath is supposed to contain the path, and does not, it would be better to fix sysPropClassPath to contain the correct value.
>>
>> -- Jon
>>
>> On 4/23/19 2:58 AM, Patrick Zhang OS wrote:
>>> Hi,
>>>
>>> I ran into an issue that jtreg reported that it cannot determine version for JDK when debugging, while the root cause is the root directory of class files could not be located correctly. There are a couple of methods to solve the problem (see details in JBS), the proposed one is to make com/sun/javatest/regtest/config/JDK.java not overwrite the CLASSPATH env var completely, update it with added paths instead.
>>> Please review, thanks.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/CODETOOLS-7902453
>>> Webrev:
>>> http://cr.openjdk.java.net/~qpzhang/codetools-7902453/webrev.01/
>>>
>>> Regards
>>> Patrick
>>>