Re: RFR 8214794 : java.specification.version should be only the major version number
Looks good to me, too. /Magnus > 4 dec. 2018 kl. 20:34 skrev Mandy Chung : > > The revised webrev looks okay. > > Mandy > >> On 12/4/18 11:32 AM, Roger Riggs wrote: >> Hi Mandy, Martin, >> >> The new test is unnecessary, the case is covered by >> java/lang/System/Versions test >> and uses the stronger comparison for the version numbers. >> >> It would not detect the problem unless the version included more than the >> major version. >> >> Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-3/ >> >> Thanks, Roger >> >>> On 12/04/2018 01:41 PM, Mandy Chung wrote: >>> >>> On 12/4/18 8:16 AM, Roger Riggs wrote: Please review correctly setting the java.specification.version property with only the major version number. A test is added to ensure the java spec version agrees with the major version. The symptoms are that jtreg would fail with a full version number. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/ >>> >>> Looks okay. I agree with Martin to go with a stronger assertion without >>> converting into a number. test/jdk/java/lang/System/Versions.java looks >>> like also covering this test case. At some point it'd be good to >>> consolidate these two tests. >>> >>> Nit: in GensrcMisc.gmk, I think VERSION_NUMBER and VERSION_PRE etc are a >>> relevant group. VERSION_SPECIFICATION can be moved to group with >>> VERSION_CLASSFILE_MAJOR and MINOR. Magnus may have an opinion. >>> >>> Mandy >> >
Re: RFR 8214794 : java.specification.version should be only the major version number
> > LGTM
Re: RFR 8214794 : java.specification.version should be only the major version number
Looks good. /Erik On 2018-12-04 11:32, Roger Riggs wrote: Hi Mandy, Martin, The new test is unnecessary, the case is covered by java/lang/System/Versions test and uses the stronger comparison for the version numbers. It would not detect the problem unless the version included more than the major version. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-3/ Thanks, Roger On 12/04/2018 01:41 PM, Mandy Chung wrote: On 12/4/18 8:16 AM, Roger Riggs wrote: Please review correctly setting the java.specification.version property with only the major version number. A test is added to ensure the java spec version agrees with the major version. The symptoms are that jtreg would fail with a full version number. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/ Looks okay. I agree with Martin to go with a stronger assertion without converting into a number. test/jdk/java/lang/System/Versions.java looks like also covering this test case. At some point it'd be good to consolidate these two tests. Nit: in GensrcMisc.gmk, I think VERSION_NUMBER and VERSION_PRE etc are a relevant group. VERSION_SPECIFICATION can be moved to group with VERSION_CLASSFILE_MAJOR and MINOR. Magnus may have an opinion. Mandy
Re: RFR 8214794 : java.specification.version should be only the major version number
Hi Mandy, Martin, The new test is unnecessary, the case is covered by java/lang/System/Versions test and uses the stronger comparison for the version numbers. It would not detect the problem unless the version included more than the major version. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-3/ Thanks, Roger On 12/04/2018 01:41 PM, Mandy Chung wrote: On 12/4/18 8:16 AM, Roger Riggs wrote: Please review correctly setting the java.specification.version property with only the major version number. A test is added to ensure the java spec version agrees with the major version. The symptoms are that jtreg would fail with a full version number. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/ Looks okay. I agree with Martin to go with a stronger assertion without converting into a number. test/jdk/java/lang/System/Versions.java looks like also covering this test case. At some point it'd be good to consolidate these two tests. Nit: in GensrcMisc.gmk, I think VERSION_NUMBER and VERSION_PRE etc are a relevant group. VERSION_SPECIFICATION can be moved to group with VERSION_CLASSFILE_MAJOR and MINOR. Magnus may have an opinion. Mandy
Re: RFR 8214794 : java.specification.version should be only the major version number
The revised webrev looks okay. Mandy On 12/4/18 11:32 AM, Roger Riggs wrote: Hi Mandy, Martin, The new test is unnecessary, the case is covered by java/lang/System/Versions test and uses the stronger comparison for the version numbers. It would not detect the problem unless the version included more than the major version. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-3/ Thanks, Roger On 12/04/2018 01:41 PM, Mandy Chung wrote: On 12/4/18 8:16 AM, Roger Riggs wrote: Please review correctly setting the java.specification.version property with only the major version number. A test is added to ensure the java spec version agrees with the major version. The symptoms are that jtreg would fail with a full version number. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/ Looks okay. I agree with Martin to go with a stronger assertion without converting into a number. test/jdk/java/lang/System/Versions.java looks like also covering this test case. At some point it'd be good to consolidate these two tests. Nit: in GensrcMisc.gmk, I think VERSION_NUMBER and VERSION_PRE etc are a relevant group. VERSION_SPECIFICATION can be moved to group with VERSION_CLASSFILE_MAJOR and MINOR. Magnus may have an opinion. Mandy
Re: RFR 8214794 : java.specification.version should be only the major version number
Hi Roger, Looks fine. Brian > On Dec 4, 2018, at 8:23 AM, Roger Riggs wrote: > > Including build-dev for the change to GensrcMisc.gmk. > > thx. > > On 12/04/2018 11:16 AM, Roger Riggs wrote: >> Please review correctly setting the java.specification.version property >> with only the major version number. A test is added to ensure the >> java spec version agrees with the major version. >> >> The symptoms are that jtreg would fail with a full version number. >> >> Webrev: >> http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2
Re: RFR 8214794 : java.specification.version should be only the major version number
On 12/4/18 8:16 AM, Roger Riggs wrote: Please review correctly setting the java.specification.version property with only the major version number. A test is added to ensure the java spec version agrees with the major version. The symptoms are that jtreg would fail with a full version number. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/ Looks okay. I agree with Martin to go with a stronger assertion without converting into a number. test/jdk/java/lang/System/Versions.java looks like also covering this test case. At some point it'd be good to consolidate these two tests. Nit: in GensrcMisc.gmk, I think VERSION_NUMBER and VERSION_PRE etc are a relevant group. VERSION_SPECIFICATION can be moved to group with VERSION_CLASSFILE_MAJOR and MINOR. Magnus may have an opinion. Mandy
Re: RFR 8214794 : java.specification.version should be only the major version number
Including build-dev for the change to GensrcMisc.gmk. thx. On 12/04/2018 11:16 AM, Roger Riggs wrote: Please review correctly setting the java.specification.version property with only the major version number. A test is added to ensure the java spec version agrees with the major version. The symptoms are that jtreg would fail with a full version number. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/ Thanks, Roger
