Re: [12]: RFR: Use CLDR's time zone mappings for Windows
On 2018-09-12 03:19, Magnus Ihse Bursie wrote: On 2018-09-10 23:34, Naoto Sato wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8209167 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8209167/webrev.01/ Some comments: In make/copy/Copy-java.base.gmk: +ifneq ($(findstring $(OPENJDK_TARGET_OS), aix),) The findstring construct is hard to read and only needed when we test more than one OS. Please rewrite as a single ifeq test for aix. In GensrcCLDR.gmk: I don't understand the CLDR_WINTZMAPPINGS file? There's no rule to generate it, there's just a dependency..? It's generated by the same rule as the other file. This is the easiest workaround for two files generated from the same rule. It sort of works (for clean builds and if the input is chagned), but won't handle all cases where one file is removed externally and the other is not. I suggested this construct to Naoto. Perhaps a comment explaining this would be good. The removal of the duplicate "common", that seems like a separate bug fix? It does indeed. Before this fix, the wildcards must have returned empty. /Erik /Magnus This fix is to remove the hand crafted map file (lib/tzmappings) on Windows, which maps Windows time zones to Java's tzid. It is now dynamically generated at the build time, using CLDR's data file (windowsZones.xml) Naoto
Re: [12]: RFR: Use CLDR's time zone mappings for Windows
On 2018-09-12 12:19, Magnus Ihse Bursie wrote: On 2018-09-10 23:34, Naoto Sato wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8209167 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8209167/webrev.01/ Oh, and there's a typo in CLDRConverter.java: " Note: the entries are alphabetically soreted, *except* the "world" region". /Magnus
Re: [12]: RFR: Use CLDR's time zone mappings for Windows
On 2018-09-10 23:34, Naoto Sato wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8209167 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8209167/webrev.01/ Some comments: In make/copy/Copy-java.base.gmk: +ifneq ($(findstring $(OPENJDK_TARGET_OS), aix),) The findstring construct is hard to read and only needed when we test more than one OS. Please rewrite as a single ifeq test for aix. In GensrcCLDR.gmk: I don't understand the CLDR_WINTZMAPPINGS file? There's no rule to generate it, there's just a dependency..? The removal of the duplicate "common", that seems like a separate bug fix? /Magnus This fix is to remove the hand crafted map file (lib/tzmappings) on Windows, which maps Windows time zones to Java's tzid. It is now dynamically generated at the build time, using CLDR's data file (windowsZones.xml) Naoto
Re: [12]: RFR: Use CLDR's time zone mappings for Windows
Looks good. /Erik On 2018-09-10 14:34, Naoto Sato wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8209167 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8209167/webrev.01/ This fix is to remove the hand crafted map file (lib/tzmappings) on Windows, which maps Windows time zones to Java's tzid. It is now dynamically generated at the build time, using CLDR's data file (windowsZones.xml) Naoto
[12]: RFR: Use CLDR's time zone mappings for Windows
Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8209167 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8209167/webrev.01/ This fix is to remove the hand crafted map file (lib/tzmappings) on Windows, which maps Windows time zones to Java's tzid. It is now dynamically generated at the build time, using CLDR's data file (windowsZones.xml) Naoto
