Hi Kumar, Thanks for looking at my change! > I looked at some of the components I maintain, and they look good. May I ask which these are? So I can account whether all parts have been reviewed?
> 1. The local variables/fields don't comply with the coding conventions, > we have been > following in the JDK. Removed all '_' from fields. Anything else? TOOLS_NOT_TO_TEST is formatted as the similar list in VersionCheck.java. > 2. Hmm, someone parked a .ini file in bin directory, later removed, > perhaps on windows simply check for .exe ? Should a check exist > to verify if file has executable permissions ?"File.canExecute". > > 171 // Returns true if the file is not a tool. > 172 static boolean notATool(String file) { > 173 file = file.toLowerCase(); > 174 if (file.endsWith(".dll") || > 175 file.endsWith(".map") || > 176 file.endsWith(".pdb") || > 177 file.equals("server")) { // server subdir on windows. > 178 return true; > 179 } > 180 return false; > 181 } I anyways wondered why .dll + friends are in the exe directory and not in the lib directory. But a check for executable file is better than this list. Changed. > 3. Typo in comment Fixed. > 4. There is a method to check for isEnglishLocale in TestHelper, perhaps > use it to have the test bale out in non english locales as early as possible ? I added a bailout at the beginning of the test. Thanks for the advice! But thinking about this a @requires englishLocale would be useful here. > 5. Has this been tested on all platforms ? do you need help testing it ? Our testing is on ntamd64, linuxppc64, linuxppc64le, linuxs390x, linuxx86_64, sun_64 and aixppc64. I run the jdk jtreg tests on these platforms. I once ran the change with the jdk-hs repo, where we test test/hotspot/jtreg and most jck tests. All passed. I don't think this is very platform dependent (most differences are between unix/windows) so this platform coverage should suffice I think. I'll post a new webrev later on. Best regards, Goetz. > > Kumar > > > On 11/17/2017 3:23 AM, Lindenmaier, Goetz wrote: > > Hi, > > > > please review this change. I also filed a CSR for this: > > http://cr.openjdk.java.net/~goetz/wr17/8189102- > helpMessage/webrev.02/ > > Bug: https://bugs.openjdk.java.net/browse/JDK-8189102 > > CSR: https://bugs.openjdk.java.net/browse/JDK-8191477 > > > > See the webrev for a detailed description of the changes. > > > > If required, I'll make break-out changes to be reviewed separately. > > > > I had posted a RFR before, but improved the change to > > give a more complete overview of currently supported flags > > for the CSR: > > http://mail.openjdk.java.net/pipermail/hotspot-dev/2017- > October/028615.html > > > > Best regards, > > Goetz. > >