Re: RFR(XS) : 8186095 : upgrade to jtreg 4.2 b08

[David Holmes]

Thanks Ioi, and Igor, for clarifying.

David


On 11/08/2017 3:23 PM, Ioi Lam wrote:



On 8/10/17 9:46 PM, David Holmes wrote:

On 11/08/2017 2:31 PM, Igor Ignatyev wrote:


On Aug 10, 2017, at 9:22 PM, David Holmes  
wrote:


Hi Igor,

On 11/08/2017 2:02 PM, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8186095/webrev.00/index.html

14 lines changed: 1 ins; 0 del; 13 mod;

Hi all,
could you please review this small patch which bumps up jtreg version?
besides updating jib profiles and all TEST.ROOT files,


That all looks fine (though deploy should not be in there).


the fix updates
hotspot/test/runtime/Metaspace/FragmentMetaspaceSimple.java test 
not to

rely on having "library" test.Empty class in 'test.classes' and put
test.Empty class in the workdir instead.


Sorry I'm not following this part. You made two changes:

1. Added @library /test/lib

/test/lib is needed for ClassFileInstaller.


Okay.



What is this doing? (For that matter what is the existing "classes" 
entry supposed to mean ??? how is "classes" a library?)
existing 'classes' is the directory in 
hotspot/test/runtime/Metaspace/ which contains source of test.Empty.


Okay.



2. Instead of the test reading from test.classes you are using the 
ClassfileInstaller to copy the class to the working directory.


How does this make a difference to anything? If the test wouldn't 
find the class in test.classes, doesn't that mean ClassfileInstaller 
will also fail to find it?
test.classes points to the directory w/ classes from a test, but not 
from test libraries. directories w/ all needed classes (either from a 
test or from libraries) are added to classpath and 'test.class.path'. 
ClassFileInstaller uses class loader to get resources, test.Empty 
will be in CP, so ClassFileInstaller will have access to it.


Sorry still don't understand the change. Where does:

@build test.Empty

place Empty.class? If not in test.classes then how has this test ever 
passed? I'm assuming the change is needed because it no longer passes 
with the updated jtreg.



Hi David,

   27  * @build test.Empty

puts test.Empty class somewhere in the classpath

   28  * @run driver ClassFileInstaller test.Empty

ClassFileInstaller uses Class.getResourceAsStream("test/Empty.class") to 
read the contents of this class (from the CLASSPATH), and writes it to 
./test/Empty.class


   55 String fileName = "test" + sep + "Empty.class";
   56 File file = new File(fileName);
   57 byte buff[] = read(file);

here the test reads "./test/Empty.class" into a buffer. (When the test 
runs, the current directory is the "scratch" directory).


The old version of this test assumed that JTREG builds the Empty.class 
into a certain location, but that has changed with the jtreg 4.2 b08 
feature.


ClassFileInstaller is the proper way of reading the bytes of class 
files, and has been used by most other test cases. 
FragmentMetaspaceSimple seems to be the only exception.


Thanks
- Ioi





Thanks,
David



Thanks,
David
-


testing: :hotspot_all, {jdk,langtools,nashorn,jaxp}/test/:tier[1-3]
Thanks,
-- Igor

Re: RFR(XS) : 8186095 : upgrade to jtreg 4.2 b08

[Ioi Lam]

On 8/10/17 9:46 PM, David Holmes wrote:

On 11/08/2017 2:31 PM, Igor Ignatyev wrote:


On Aug 10, 2017, at 9:22 PM, David Holmes  
wrote:


Hi Igor,

On 11/08/2017 2:02 PM, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8186095/webrev.00/index.html

14 lines changed: 1 ins; 0 del; 13 mod;

Hi all,
could you please review this small patch which bumps up jtreg version?
besides updating jib profiles and all TEST.ROOT files,


That all looks fine (though deploy should not be in there).


the fix updates
hotspot/test/runtime/Metaspace/FragmentMetaspaceSimple.java test 
not to

rely on having "library" test.Empty class in 'test.classes' and put
test.Empty class in the workdir instead.


Sorry I'm not following this part. You made two changes:

1. Added @library /test/lib

/test/lib is needed for ClassFileInstaller.


Okay.



What is this doing? (For that matter what is the existing "classes" 
entry supposed to mean ??? how is "classes" a library?)
existing 'classes' is the directory in 
hotspot/test/runtime/Metaspace/ which contains source of test.Empty.


Okay.



2. Instead of the test reading from test.classes you are using the 
ClassfileInstaller to copy the class to the working directory.


How does this make a difference to anything? If the test wouldn't 
find the class in test.classes, doesn't that mean ClassfileInstaller 
will also fail to find it?
test.classes points to the directory w/ classes from a test, but not 
from test libraries. directories w/ all needed classes (either from a 
test or from libraries) are added to classpath and 'test.class.path'. 
ClassFileInstaller uses class loader to get resources, test.Empty 
will be in CP, so ClassFileInstaller will have access to it.


Sorry still don't understand the change. Where does:

@build test.Empty

place Empty.class? If not in test.classes then how has this test ever 
passed? I'm assuming the change is needed because it no longer passes 
with the updated jtreg.



Hi David,

  27  * @build test.Empty

puts test.Empty class somewhere in the classpath

  28  * @run driver ClassFileInstaller test.Empty

ClassFileInstaller uses Class.getResourceAsStream("test/Empty.class") to 
read the contents of this class (from the CLASSPATH), and writes it to 
./test/Empty.class


  55 String fileName = "test" + sep + "Empty.class";
  56 File file = new File(fileName);
  57 byte buff[] = read(file);

here the test reads "./test/Empty.class" into a buffer. (When the test 
runs, the current directory is the "scratch" directory).


The old version of this test assumed that JTREG builds the Empty.class 
into a certain location, but that has changed with the jtreg 4.2 b08 
feature.


ClassFileInstaller is the proper way of reading the bytes of class 
files, and has been used by most other test cases. 
FragmentMetaspaceSimple seems to be the only exception.


Thanks
- Ioi





Thanks,
David



Thanks,
David
-


testing: :hotspot_all, {jdk,langtools,nashorn,jaxp}/test/:tier[1-3]
Thanks,
-- Igor

Re: RFR(XS) : 8186095 : upgrade to jtreg 4.2 b08

[Igor Ignatyev]

> On Aug 10, 2017, at 9:46 PM, David Holmes  wrote:
> 
> On 11/08/2017 2:31 PM, Igor Ignatyev wrote:
>>> On Aug 10, 2017, at 9:22 PM, David Holmes  wrote:
>>> 
>>> Hi Igor,
>>> 
>>> On 11/08/2017 2:02 PM, Igor Ignatyev wrote:
 http://cr.openjdk.java.net/~iignatyev//8186095/webrev.00/index.html
> 14 lines changed: 1 ins; 0 del; 13 mod;
 Hi all,
 could you please review this small patch which bumps up jtreg version?
 besides updating jib profiles and all TEST.ROOT files,
>>> 
>>> That all looks fine (though deploy should not be in there).
>>> 
 the fix updates
 hotspot/test/runtime/Metaspace/FragmentMetaspaceSimple.java test not to
 rely on having "library" test.Empty class in 'test.classes' and put
 test.Empty class in the workdir instead.
>>> 
>>> Sorry I'm not following this part. You made two changes:
>>> 
>>> 1. Added @library /test/lib
>> /test/lib is needed for ClassFileInstaller.
> 
> Okay.
> 
>>> 
>>> What is this doing? (For that matter what is the existing "classes" entry 
>>> supposed to mean ??? how is "classes" a library?)
>> existing 'classes' is the directory in hotspot/test/runtime/Metaspace/ which 
>> contains source of test.Empty.
> 
> Okay.
> 
>>> 
>>> 2. Instead of the test reading from test.classes you are using the 
>>> ClassfileInstaller to copy the class to the working directory.
>>> 
>>> How does this make a difference to anything? If the test wouldn't find the 
>>> class in test.classes, doesn't that mean ClassfileInstaller will also fail 
>>> to find it?
>> test.classes points to the directory w/ classes from a test, but not from 
>> test libraries. directories w/ all needed classes (either from a test or 
>> from libraries) are added to classpath and 'test.class.path'. 
>> ClassFileInstaller uses class loader to get resources, test.Empty will be in 
>> CP, so ClassFileInstaller will have access to it.
> 
> Sorry still don't understand the change. Where does:
> 
> @build test.Empty
> 
> place Empty.class? If not in test.classes then how has this test ever passed? 
> I'm assuming the change is needed because it no longer passes with the 
> updated jtreg.
build places it in a library dedicated directory, in this case it will be 
'JTwork/classes//runtime/Metaspace/classes', but 'test.classes' points to 
'JTwork/classes//runtime/Metaspace/FragmentMetaspaceSimple.d'.  
'test.class.path' will have both (separated by path separator), classpath has 
these two paths and couple others. 
> 
> Thanks,
> David
> 
>>> 
>>> Thanks,
>>> David
>>> -
>>> 
 testing: :hotspot_all, {jdk,langtools,nashorn,jaxp}/test/:tier[1-3]
 Thanks,
 -- Igor

Re: RFR(XS) : 8186095 : upgrade to jtreg 4.2 b08

[David Holmes]

On 11/08/2017 2:31 PM, Igor Ignatyev wrote:



On Aug 10, 2017, at 9:22 PM, David Holmes  wrote:

Hi Igor,

On 11/08/2017 2:02 PM, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8186095/webrev.00/index.html

14 lines changed: 1 ins; 0 del; 13 mod;

Hi all,
could you please review this small patch which bumps up jtreg version?
besides updating jib profiles and all TEST.ROOT files,


That all looks fine (though deploy should not be in there).


the fix updates
hotspot/test/runtime/Metaspace/FragmentMetaspaceSimple.java test not to
rely on having "library" test.Empty class in 'test.classes' and put
test.Empty class in the workdir instead.


Sorry I'm not following this part. You made two changes:

1. Added @library /test/lib

/test/lib is needed for ClassFileInstaller.


Okay.



What is this doing? (For that matter what is the existing "classes" entry supposed to 
mean ??? how is "classes" a library?)

existing 'classes' is the directory in hotspot/test/runtime/Metaspace/ which 
contains source of test.Empty.


Okay.



2. Instead of the test reading from test.classes you are using the 
ClassfileInstaller to copy the class to the working directory.

How does this make a difference to anything? If the test wouldn't find the 
class in test.classes, doesn't that mean ClassfileInstaller will also fail to 
find it?

test.classes points to the directory w/ classes from a test, but not from test 
libraries. directories w/ all needed classes (either from a test or from 
libraries) are added to classpath and 'test.class.path'. ClassFileInstaller 
uses class loader to get resources, test.Empty will be in CP, so 
ClassFileInstaller will have access to it.


Sorry still don't understand the change. Where does:

@build test.Empty

place Empty.class? If not in test.classes then how has this test ever 
passed? I'm assuming the change is needed because it no longer passes 
with the updated jtreg.


Thanks,
David



Thanks,
David
-


testing: :hotspot_all, {jdk,langtools,nashorn,jaxp}/test/:tier[1-3]
Thanks,
-- Igor

Re: RFR(XS) : 8186095 : upgrade to jtreg 4.2 b08

[Igor Ignatyev]

> On Aug 10, 2017, at 9:22 PM, David Holmes  wrote:
> 
> Hi Igor,
> 
> On 11/08/2017 2:02 PM, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8186095/webrev.00/index.html
>>> 14 lines changed: 1 ins; 0 del; 13 mod;
>> Hi all,
>> could you please review this small patch which bumps up jtreg version?
>> besides updating jib profiles and all TEST.ROOT files, 
> 
> That all looks fine (though deploy should not be in there).
> 
>> the fix updates
>> hotspot/test/runtime/Metaspace/FragmentMetaspaceSimple.java test not to
>> rely on having "library" test.Empty class in 'test.classes' and put
>> test.Empty class in the workdir instead.
> 
> Sorry I'm not following this part. You made two changes:
> 
> 1. Added @library /test/lib
/test/lib is needed for ClassFileInstaller.
> 
> What is this doing? (For that matter what is the existing "classes" entry 
> supposed to mean ??? how is "classes" a library?)
existing 'classes' is the directory in hotspot/test/runtime/Metaspace/ which 
contains source of test.Empty.
> 
> 2. Instead of the test reading from test.classes you are using the 
> ClassfileInstaller to copy the class to the working directory.
> 
> How does this make a difference to anything? If the test wouldn't find the 
> class in test.classes, doesn't that mean ClassfileInstaller will also fail to 
> find it?
test.classes points to the directory w/ classes from a test, but not from test 
libraries. directories w/ all needed classes (either from a test or from 
libraries) are added to classpath and 'test.class.path'. ClassFileInstaller 
uses class loader to get resources, test.Empty will be in CP, so 
ClassFileInstaller will have access to it.
> 
> Thanks,
> David
> -
> 
>> testing: :hotspot_all, {jdk,langtools,nashorn,jaxp}/test/:tier[1-3]
>> Thanks,
>> -- Igor

Re: RFR(XS) : 8186095 : upgrade to jtreg 4.2 b08

[David Holmes]

Hi Igor,

On 11/08/2017 2:02 PM, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8186095/webrev.00/index.html

14 lines changed: 1 ins; 0 del; 13 mod;


Hi all,

could you please review this small patch which bumps up jtreg version?
besides updating jib profiles and all TEST.ROOT files, 


That all looks fine (though deploy should not be in there).


the fix updates
hotspot/test/runtime/Metaspace/FragmentMetaspaceSimple.java test not to
rely on having "library" test.Empty class in 'test.classes' and put
test.Empty class in the workdir instead.


Sorry I'm not following this part. You made two changes:

1. Added @library /test/lib

What is this doing? (For that matter what is the existing "classes" 
entry supposed to mean ??? how is "classes" a library?)


2. Instead of the test reading from test.classes you are using the 
ClassfileInstaller to copy the class to the working directory.


How does this make a difference to anything? If the test wouldn't find 
the class in test.classes, doesn't that mean ClassfileInstaller will 
also fail to find it?


Thanks,
David
-


testing: :hotspot_all, {jdk,langtools,nashorn,jaxp}/test/:tier[1-3]

Thanks,
-- Igor

RFR(XS) : 8186095 : upgrade to jtreg 4.2 b08

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8186095/webrev.00/index.html
> 14 lines changed: 1 ins; 0 del; 13 mod; 

Hi all,

could you please review this small patch which bumps up jtreg version?
besides updating jib profiles and all TEST.ROOT files, the fix updates 
hotspot/test/runtime/Metaspace/FragmentMetaspaceSimple.java test not to rely on 
having "library" test.Empty class in 'test.classes' and put test.Empty class in 
the workdir instead.

testing: :hotspot_all, {jdk,langtools,nashorn,jaxp}/test/:tier[1-3]

Thanks,
-- Igor 

Re: RFR: JDK-8186052: Fix a11y and HTML issues in the java.base/java.lang[.*] packages

[Jonathan Gibbons]

Update: addresses Stuart's comment for String.split

Webrev: http://cr.openjdk.java.net/~jjg/8186052/webrev.01/index.html
API: http://cr.openjdk.java.net/~jjg/8186052/api.01/index.html

-- Jon

On 08/09/2017 06:28 PM, Stuart Marks wrote:

On 8/9/17 5:11 PM, Jonathan Gibbons wrote:
Please review this proposed change to address accessibility and HTML 
issues

in the java.base java.lang[.*] packages.

In general, the changes consist of
* update tables to add scope=row|col as appropriate.
In some cases, slightly more surgery was needed on the table
* fix issues reported by tidy
In most cases, this is removing superfluous  elements.

Webrev: http://cr.openjdk.java.net/~jjg/8186052/webrev.00/index.html
API: http://cr.openjdk.java.net/~jjg/8186052/api.00/index.html


Hi Jon,

These changes look good. In most cases the table surgery was a big 
improvement. One small quibble I have is with the table in 
String.split(regex, limit). The limit column has numbers, and the 
alignment changed from centered to left-aligned, neither of which is 
really very good. I'd suggest changing this column to be 
right-aligned, so the negative values in this column will be aligned 
appropriately.


Thanks,

s'marks

Re: RFR: JDK-8185994: Fix a11y and HTML issues in the java.base/java.io and java.base/java.nio packages

[Brian Burkhalter]

I think it looks fine now.

Brian

On Aug 10, 2017, at 1:59 PM, Jonathan Gibbons  
wrote:

> I have posted an update to the webrev and API.
> 
> The change is in DataInput ... I have tweaked the position of the text in the 
> cells of that initial table.
> I would prefer not to iterate further on the layout of this table, since the 
> changeset is primarily
> about addressing accessibility and HTML correctness issues. 

Re: RFR: JDK-8185994: Fix a11y and HTML issues in the java.base/java.io and java.base/java.nio packages

[Jonathan Gibbons]

Ooops ... forgot the updated links:

Webrev: http://cr.openjdk.java.net/~jjg/8185994/webrev.01/index.html
API: http://cr.openjdk.java.net/~jjg/8185994/api.01/index.html

-- Jon

On 08/10/2017 01:59 PM, Jonathan Gibbons wrote:

I have posted an update to the webrev and API.

The change is in DataInput ... I have tweaked the position of the text 
in the cells of that initial table.
I would prefer not to iterate further on the layout of this table, 
since the changeset is primarily

about addressing accessibility and HTML correctness issues.

-- Jon


On 08/10/2017 01:00 PM, Brian Burkhalter wrote:
On Aug 9, 2017, at 6:56 PM, Stuart Marks > wrote:



On 8/9/17 5:14 PM, Jonathan Gibbons wrote:
Please review this proposed change to address accessibility and 
HTML issues

in the java.base java.io and java.nio[.*] packages.

In general, the changes consist of
* update tables to add scope=row|col as appropriate.
   In some cases, slightly more surgery was needed on the table
* fix issues reported by tidy
   In most cases, this is removing superfluous  elements.

Webrev:http://cr.openjdk.java.net/~jjg/8185994/webrev.00/index.html 

API:http://cr.openjdk.java.net/~jjg/8185994/api.00/index.html 



Again, changes mostly look good. One small formatting point on 
DataInput. The numbers in the table should all have similar 
alignment. Most of the numbers in the table body look centered. I 
think it would look better if the numbers in the "Bit Values" column 
headers, and the numbers in the Byte column, were all centered. 
Since they're all single digits, you can probably get away with 
centering (unless you want to change them all to right-aligned with 
padding, or something).


I concur with Stuart that this is the only problem area. Centering 
the Byte column would be good for sure. Centering the “Bit Values” 
entries “bits x-y” could work if they were aligned in a visually 
appealing manner, e.g., centered with respect to the same vertical. 
Another possibility would be to have a separate cell for each 
row-column position and put “b6”, “b5”, etc. in the cells perhaps 
with a footnote below the table about the notation unless it’s deemed 
obvious (maybe use subscript elements b6, etc.?).


Brian

Re: RFR: JDK-8185994: Fix a11y and HTML issues in the java.base/java.io and java.base/java.nio packages

[Jonathan Gibbons]

I have posted an update to the webrev and API.

The change is in DataInput ... I have tweaked the position of the text 
in the cells of that initial table.
I would prefer not to iterate further on the layout of this table, since 
the changeset is primarily

about addressing accessibility and HTML correctness issues.

-- Jon


On 08/10/2017 01:00 PM, Brian Burkhalter wrote:
On Aug 9, 2017, at 6:56 PM, Stuart Marks > wrote:



On 8/9/17 5:14 PM, Jonathan Gibbons wrote:
Please review this proposed change to address accessibility and HTML 
issues

in the java.base java.io and java.nio[.*] packages.

In general, the changes consist of
* update tables to add scope=row|col as appropriate.
   In some cases, slightly more surgery was needed on the table
* fix issues reported by tidy
   In most cases, this is removing superfluous  elements.

Webrev:http://cr.openjdk.java.net/~jjg/8185994/webrev.00/index.html 

API:http://cr.openjdk.java.net/~jjg/8185994/api.00/index.html 



Again, changes mostly look good. One small formatting point on 
DataInput. The numbers in the table should all have similar 
alignment. Most of the numbers in the table body look centered. I 
think it would look better if the numbers in the "Bit Values" column 
headers, and the numbers in the Byte column, were all centered. Since 
they're all single digits, you can probably get away with centering 
(unless you want to change them all to right-aligned with padding, or 
something).


I concur with Stuart that this is the only problem area. Centering the 
Byte column would be good for sure. Centering the “Bit Values” entries 
“bits x-y” could work if they were aligned in a visually appealing 
manner, e.g., centered with respect to the same vertical. Another 
possibility would be to have a separate cell for each row-column 
position and put “b6”, “b5”, etc. in the cells perhaps with a footnote 
below the table about the notation unless it’s deemed obvious (maybe 
use subscript elements b6, etc.?).


Brian

Re: RFR: JDK-8185994: Fix a11y and HTML issues in the java.base/java.io and java.base/java.nio packages

[Brian Burkhalter]

On Aug 9, 2017, at 6:56 PM, Stuart Marks  wrote:

> On 8/9/17 5:14 PM, Jonathan Gibbons wrote:
>> Please review this proposed change to address accessibility and HTML issues
>> in the java.base java.io and java.nio[.*] packages.
>> 
>> In general, the changes consist of
>> * update tables to add scope=row|col as appropriate.
>>In some cases, slightly more surgery was needed on the table
>> * fix issues reported by tidy
>>In most cases, this is removing superfluous  elements.
>> 
>> Webrev: http://cr.openjdk.java.net/~jjg/8185994/webrev.00/index.html
>> API: http://cr.openjdk.java.net/~jjg/8185994/api.00/index.html
> 
> Again, changes mostly look good. One small formatting point on DataInput. The 
> numbers in the table should all have similar alignment. Most of the numbers 
> in the table body look centered. I think it would look better if the numbers 
> in the "Bit Values" column headers, and the numbers in the Byte column, were 
> all centered. Since they're all single digits, you can probably get away with 
> centering (unless you want to change them all to right-aligned with padding, 
> or something).

I concur with Stuart that this is the only problem area. Centering the Byte 
column would be good for sure. Centering the “Bit Values” entries “bits x-y” 
could work if they were aligned in a visually appealing manner, e.g., centered 
with respect to the same vertical. Another possibility would be to have a 
separate cell for each row-column position and put “b6”, “b5”, etc. in the 
cells perhaps with a footnote below the table about the notation unless it’s 
deemed obvious (maybe use subscript elements b6, etc.?).

Brian

Re: JDK 9 jar tool issues - "isolated nested class" and creation of a corrupt jar

[Tom Hood]

Glad you were able to reproduce it.  Thanks for creating the JBS issue.

On Thu, Aug 10, 2017 at 11:49 AM, mandy chung 
wrote:

>
>
> On 8/9/17 4:50 PM, Tom Hood wrote:
>
>> Hi,
>>
>> I'm having a couple issues with the jar tool. Version: JDK 9 build 181
>> SPARC64
>>
>> Since I'm typing this in, beware of typos.  I cannot copy and paste output
>> as my Solaris 10 box isn't permitted to access the internet.
>>
>> (1) isolated nested classes trying to create a multi-release jar
>>
>
> I can reproduce the isolated nested class error.   It seems a bug in how
> it validates a nested class when its enclosing class is a nested class.  I
> create a JBS issue to track it:
>https://bugs.openjdk.java.net/browse/JDK-8186087
>
> We will look into it and provide answers to the ordering of the entry list
> and also corrupted JAR on sparc64.
>
> Mandy
>

Re: JDK 9 jar tool issues - "isolated nested class" and creation of a corrupt jar

[mandy chung]

On 8/9/17 4:50 PM, Tom Hood wrote:

Hi,

I'm having a couple issues with the jar tool. Version: JDK 9 build 181
SPARC64

Since I'm typing this in, beware of typos.  I cannot copy and paste output
as my Solaris 10 box isn't permitted to access the internet.

(1) isolated nested classes trying to create a multi-release jar


I can reproduce the isolated nested class error.   It seems a bug in how 
it validates a nested class when its enclosing class is a nested class.  
I create a JBS issue to track it:

   https://bugs.openjdk.java.net/browse/JDK-8186087

We will look into it and provide answers to the ordering of the entry 
list and also corrupted JAR on sparc64.


Mandy