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.