Re: [9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base
Thanks. Created an issue for the follow-on issue: https://bugs.openjdk.java.net/browse/JDK-8165804 Naoto On 9/9/16 3:34 PM, Mandy Chung wrote: On Sep 9, 2016, at 2:58 PM, Naoto Sato wrote: While at it, I noticed two build issues and updated the webrev. They are not directly related to the split package issue per se, but related to Thai breakiterator: 1) BreakIteratorRules_th.class slipped into the product image, which shouldn't. 2) Removed BreakIteratorRulesProvider.java which is not needed, as the class above is only used at the build time and not through module. Here is the updated webrev: http://cr.openjdk.java.net/~naoto/8165605/webrev.03/ Looks fine. It’d be good to file a follow-on issue and investigate what it takes to implement Masayoshi’s suggestion - have BreakIterators::getObject to return dictionary data instead of a resource name. Mandy
Re: [9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base
> On Sep 9, 2016, at 2:58 PM, Naoto Sato wrote: > > While at it, I noticed two build issues and updated the webrev. They are not > directly related to the split package issue per se, but related to Thai > breakiterator: > > 1) BreakIteratorRules_th.class slipped into the product image, which > shouldn't. > > 2) Removed BreakIteratorRulesProvider.java which is not needed, as the class > above is only used at the build time and not through module. > > Here is the updated webrev: > > http://cr.openjdk.java.net/~naoto/8165605/webrev.03/ Looks fine. It’d be good to file a follow-on issue and investigate what it takes to implement Masayoshi’s suggestion - have BreakIterators::getObject to return dictionary data instead of a resource name. Mandy
Re: [9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base
While at it, I noticed two build issues and updated the webrev. They are not directly related to the split package issue per se, but related to Thai breakiterator: 1) BreakIteratorRules_th.class slipped into the product image, which shouldn't. 2) Removed BreakIteratorRulesProvider.java which is not needed, as the class above is only used at the build time and not through module. Here is the updated webrev: http://cr.openjdk.java.net/~naoto/8165605/webrev.03/ Naoto On 9/9/16 7:15 AM, Erik Joelsson wrote: Build change looks ok. /Erik On 2016-09-08 17:37, Naoto Sato wrote: Updated the webrev wrt the latter comment: http://cr.openjdk.java.net/~naoto/8165605/webrev.02/ Naoto On 9/7/16 6:37 PM, Mandy Chung wrote: On Sep 7, 2016, at 6:29 PM, Naoto Sato wrote: Hi Mandy, Although avoiding the hardcoded pathname is good, it is specific to the BreakIterator implementation of the COMPAT provider. So I am not sure making a generic SPI would be needed here. I was thinking of one of the internal services that jdk.localedata currently provides. Anyway, this split package issue is blocking Alan's push, so I'd like to push the change as it is. We can get back to this later. I agree this can be cleaned up as a separate issue. 152 InputStream is = module.getResourceAsStream( 153 ("jdk.localedata".equals(module.getName()) ? 154 "sun/text/resources/ext/" : "sun/text/resources/") + dictionaryName); It may be easier to read if line 153-154 are moved and assign to a separate variable. Otherwise, looks fine. Mandy Naoto On 9/7/16 5:17 PM, Mandy Chung wrote: Hi Naoto, Is there an alternative to get back the pathname of the resource e.g. adding a method in existing internal SPI to avoid hardcoding the module name and the resource pathname. Mandy On Sep 7, 2016, at 3:56 PM, Naoto Sato wrote: Forgot to include jlink plugin changes. Here is the updated webrev: http://cr.openjdk.java.net/~naoto/8165605/webrev.01/ Naoto On 9/7/16 3:03 PM, Naoto Sato wrote: Please review the changes to the subject bug: https://bugs.openjdk.java.net/browse/JDK-8165605 The proposed fix is located at: http://cr.openjdk.java.net/~naoto/8165605/webrev.00/ The change is simply to move those 3 resources under sun.text.resources.ext package so that it won't cause the split package issue. Naoto
Re: [9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base
The build tool is getting the output directory as a parameter and that also need to be tweaked. That output directory path is specified in the makefile which I wanted to avoid. Naoto On 9/8/16 4:31 PM, Masayoshi Okutsu wrote: Is it just a matter of an extra step, new File(path).getName()? Masayoshi On 9/9/2016 12:00 AM, Naoto Sato wrote: Well, actually I tried this approach first, then found out that the data files are also used by the GenerateBreakIteratorData build tool which has some implicit assumption of the value being the file name w/o path. So I ended up with the fix. Naoto On 9/8/16 5:46 AM, Alan Bateman wrote: On 08/09/2016 03:51, Masayoshi Okutsu wrote: I thought Mandy suggested that the dictionary names in a ResourceBundle contain path names rather than base names, something like this: In BreakIteratorInfo_th.java: {"WordDictionary", "thai_dict"}, to {"WordDictionary", "sun/text/resources/ext/thai_dict"}, I haven't checked any side effects of this change, though. That would be cleaner, assuming it doesn't cause issues. -Alan
Re: [9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base
Build change looks ok. /Erik On 2016-09-08 17:37, Naoto Sato wrote: Updated the webrev wrt the latter comment: http://cr.openjdk.java.net/~naoto/8165605/webrev.02/ Naoto On 9/7/16 6:37 PM, Mandy Chung wrote: On Sep 7, 2016, at 6:29 PM, Naoto Sato wrote: Hi Mandy, Although avoiding the hardcoded pathname is good, it is specific to the BreakIterator implementation of the COMPAT provider. So I am not sure making a generic SPI would be needed here. I was thinking of one of the internal services that jdk.localedata currently provides. Anyway, this split package issue is blocking Alan's push, so I'd like to push the change as it is. We can get back to this later. I agree this can be cleaned up as a separate issue. 152 InputStream is = module.getResourceAsStream( 153 ("jdk.localedata".equals(module.getName()) ? 154 "sun/text/resources/ext/" : "sun/text/resources/") + dictionaryName); It may be easier to read if line 153-154 are moved and assign to a separate variable. Otherwise, looks fine. Mandy Naoto On 9/7/16 5:17 PM, Mandy Chung wrote: Hi Naoto, Is there an alternative to get back the pathname of the resource e.g. adding a method in existing internal SPI to avoid hardcoding the module name and the resource pathname. Mandy On Sep 7, 2016, at 3:56 PM, Naoto Sato wrote: Forgot to include jlink plugin changes. Here is the updated webrev: http://cr.openjdk.java.net/~naoto/8165605/webrev.01/ Naoto On 9/7/16 3:03 PM, Naoto Sato wrote: Please review the changes to the subject bug: https://bugs.openjdk.java.net/browse/JDK-8165605 The proposed fix is located at: http://cr.openjdk.java.net/~naoto/8165605/webrev.00/ The change is simply to move those 3 resources under sun.text.resources.ext package so that it won't cause the split package issue. Naoto
Re: [9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base
Is it just a matter of an extra step, new File(path).getName()? Masayoshi On 9/9/2016 12:00 AM, Naoto Sato wrote: Well, actually I tried this approach first, then found out that the data files are also used by the GenerateBreakIteratorData build tool which has some implicit assumption of the value being the file name w/o path. So I ended up with the fix. Naoto On 9/8/16 5:46 AM, Alan Bateman wrote: On 08/09/2016 03:51, Masayoshi Okutsu wrote: I thought Mandy suggested that the dictionary names in a ResourceBundle contain path names rather than base names, something like this: In BreakIteratorInfo_th.java: {"WordDictionary", "thai_dict"}, to {"WordDictionary", "sun/text/resources/ext/thai_dict"}, I haven't checked any side effects of this change, though. That would be cleaner, assuming it doesn't cause issues. -Alan
Re: [9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base
Updated the webrev wrt the latter comment: http://cr.openjdk.java.net/~naoto/8165605/webrev.02/ Naoto On 9/7/16 6:37 PM, Mandy Chung wrote: On Sep 7, 2016, at 6:29 PM, Naoto Sato wrote: Hi Mandy, Although avoiding the hardcoded pathname is good, it is specific to the BreakIterator implementation of the COMPAT provider. So I am not sure making a generic SPI would be needed here. I was thinking of one of the internal services that jdk.localedata currently provides. Anyway, this split package issue is blocking Alan's push, so I'd like to push the change as it is. We can get back to this later. I agree this can be cleaned up as a separate issue. 152 InputStream is = module.getResourceAsStream( 153 ("jdk.localedata".equals(module.getName()) ? 154 "sun/text/resources/ext/" : "sun/text/resources/") + dictionaryName); It may be easier to read if line 153-154 are moved and assign to a separate variable. Otherwise, looks fine. Mandy Naoto On 9/7/16 5:17 PM, Mandy Chung wrote: Hi Naoto, Is there an alternative to get back the pathname of the resource e.g. adding a method in existing internal SPI to avoid hardcoding the module name and the resource pathname. Mandy On Sep 7, 2016, at 3:56 PM, Naoto Sato wrote: Forgot to include jlink plugin changes. Here is the updated webrev: http://cr.openjdk.java.net/~naoto/8165605/webrev.01/ Naoto On 9/7/16 3:03 PM, Naoto Sato wrote: Please review the changes to the subject bug: https://bugs.openjdk.java.net/browse/JDK-8165605 The proposed fix is located at: http://cr.openjdk.java.net/~naoto/8165605/webrev.00/ The change is simply to move those 3 resources under sun.text.resources.ext package so that it won't cause the split package issue. Naoto
Re: [9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base
Well, actually I tried this approach first, then found out that the data files are also used by the GenerateBreakIteratorData build tool which has some implicit assumption of the value being the file name w/o path. So I ended up with the fix. Naoto On 9/8/16 5:46 AM, Alan Bateman wrote: On 08/09/2016 03:51, Masayoshi Okutsu wrote: I thought Mandy suggested that the dictionary names in a ResourceBundle contain path names rather than base names, something like this: In BreakIteratorInfo_th.java: {"WordDictionary", "thai_dict"}, to {"WordDictionary", "sun/text/resources/ext/thai_dict"}, I haven't checked any side effects of this change, though. That would be cleaner, assuming it doesn't cause issues. -Alan
Re: [9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base
On 08/09/2016 03:51, Masayoshi Okutsu wrote: I thought Mandy suggested that the dictionary names in a ResourceBundle contain path names rather than base names, something like this: In BreakIteratorInfo_th.java: {"WordDictionary", "thai_dict"}, to {"WordDictionary", "sun/text/resources/ext/thai_dict"}, I haven't checked any side effects of this change, though. That would be cleaner, assuming it doesn't cause issues. -Alan
Re: [9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base
I thought Mandy suggested that the dictionary names in a ResourceBundle contain path names rather than base names, something like this: In BreakIteratorInfo_th.java: {"WordDictionary", "thai_dict"}, to {"WordDictionary", "sun/text/resources/ext/thai_dict"}, I haven't checked any side effects of this change, though. I still prefer that a ResourceBundle for BreakIterators returns dictionary data by calling getObject(key). But it'd be a bit late to make that change for JDK 9. Thanks, Masayoshi On 9/8/2016 10:37 AM, Mandy Chung wrote: On Sep 7, 2016, at 6:29 PM, Naoto Sato wrote: Hi Mandy, Although avoiding the hardcoded pathname is good, it is specific to the BreakIterator implementation of the COMPAT provider. So I am not sure making a generic SPI would be needed here. I was thinking of one of the internal services that jdk.localedata currently provides. Anyway, this split package issue is blocking Alan's push, so I'd like to push the change as it is. We can get back to this later. I agree this can be cleaned up as a separate issue. 152 InputStream is = module.getResourceAsStream( 153 ("jdk.localedata".equals(module.getName()) ? 154 "sun/text/resources/ext/" : "sun/text/resources/") + dictionaryName); It may be easier to read if line 153-154 are moved and assign to a separate variable. Otherwise, looks fine. Mandy Naoto On 9/7/16 5:17 PM, Mandy Chung wrote: Hi Naoto, Is there an alternative to get back the pathname of the resource e.g. adding a method in existing internal SPI to avoid hardcoding the module name and the resource pathname. Mandy On Sep 7, 2016, at 3:56 PM, Naoto Sato wrote: Forgot to include jlink plugin changes. Here is the updated webrev: http://cr.openjdk.java.net/~naoto/8165605/webrev.01/ Naoto On 9/7/16 3:03 PM, Naoto Sato wrote: Please review the changes to the subject bug: https://bugs.openjdk.java.net/browse/JDK-8165605 The proposed fix is located at: http://cr.openjdk.java.net/~naoto/8165605/webrev.00/ The change is simply to move those 3 resources under sun.text.resources.ext package so that it won't cause the split package issue. Naoto
Re: [9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base
> On Sep 7, 2016, at 6:29 PM, Naoto Sato wrote: > > Hi Mandy, > > Although avoiding the hardcoded pathname is good, it is specific to the > BreakIterator implementation of the COMPAT provider. So I am not sure making > a generic SPI would be needed here. I was thinking of one of the internal services that jdk.localedata currently provides. > > Anyway, this split package issue is blocking Alan's push, so I'd like to push > the change as it is. We can get back to this later. I agree this can be cleaned up as a separate issue. 152 InputStream is = module.getResourceAsStream( 153 ("jdk.localedata".equals(module.getName()) ? 154 "sun/text/resources/ext/" : "sun/text/resources/") + dictionaryName); It may be easier to read if line 153-154 are moved and assign to a separate variable. Otherwise, looks fine. Mandy > > Naoto > > On 9/7/16 5:17 PM, Mandy Chung wrote: >> Hi Naoto, >> >> Is there an alternative to get back the pathname of the resource e.g. adding >> a method in existing internal SPI to avoid hardcoding the module name and >> the resource pathname. >> >> Mandy >> >>> On Sep 7, 2016, at 3:56 PM, Naoto Sato wrote: >>> >>> Forgot to include jlink plugin changes. Here is the updated webrev: >>> >>> http://cr.openjdk.java.net/~naoto/8165605/webrev.01/ >>> >>> Naoto >>> >>> On 9/7/16 3:03 PM, Naoto Sato wrote: Please review the changes to the subject bug: https://bugs.openjdk.java.net/browse/JDK-8165605 The proposed fix is located at: http://cr.openjdk.java.net/~naoto/8165605/webrev.00/ The change is simply to move those 3 resources under sun.text.resources.ext package so that it won't cause the split package issue. Naoto >>
Re: [9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base
Hi Mandy, Although avoiding the hardcoded pathname is good, it is specific to the BreakIterator implementation of the COMPAT provider. So I am not sure making a generic SPI would be needed here. Anyway, this split package issue is blocking Alan's push, so I'd like to push the change as it is. We can get back to this later. Naoto On 9/7/16 5:17 PM, Mandy Chung wrote: Hi Naoto, Is there an alternative to get back the pathname of the resource e.g. adding a method in existing internal SPI to avoid hardcoding the module name and the resource pathname. Mandy On Sep 7, 2016, at 3:56 PM, Naoto Sato wrote: Forgot to include jlink plugin changes. Here is the updated webrev: http://cr.openjdk.java.net/~naoto/8165605/webrev.01/ Naoto On 9/7/16 3:03 PM, Naoto Sato wrote: Please review the changes to the subject bug: https://bugs.openjdk.java.net/browse/JDK-8165605 The proposed fix is located at: http://cr.openjdk.java.net/~naoto/8165605/webrev.00/ The change is simply to move those 3 resources under sun.text.resources.ext package so that it won't cause the split package issue. Naoto
Re: [9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base
Hi Naoto, Is there an alternative to get back the pathname of the resource e.g. adding a method in existing internal SPI to avoid hardcoding the module name and the resource pathname. Mandy > On Sep 7, 2016, at 3:56 PM, Naoto Sato wrote: > > Forgot to include jlink plugin changes. Here is the updated webrev: > > http://cr.openjdk.java.net/~naoto/8165605/webrev.01/ > > Naoto > > On 9/7/16 3:03 PM, Naoto Sato wrote: >> Please review the changes to the subject bug: >> >> https://bugs.openjdk.java.net/browse/JDK-8165605 >> >> The proposed fix is located at: >> >> http://cr.openjdk.java.net/~naoto/8165605/webrev.00/ >> >> The change is simply to move those 3 resources under >> sun.text.resources.ext package so that it won't cause the split package >> issue. >> >> Naoto
Re: [9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base
Forgot to include jlink plugin changes. Here is the updated webrev: http://cr.openjdk.java.net/~naoto/8165605/webrev.01/ Naoto On 9/7/16 3:03 PM, Naoto Sato wrote: Please review the changes to the subject bug: https://bugs.openjdk.java.net/browse/JDK-8165605 The proposed fix is located at: http://cr.openjdk.java.net/~naoto/8165605/webrev.00/ The change is simply to move those 3 resources under sun.text.resources.ext package so that it won't cause the split package issue. Naoto
[9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base
Please review the changes to the subject bug: https://bugs.openjdk.java.net/browse/JDK-8165605 The proposed fix is located at: http://cr.openjdk.java.net/~naoto/8165605/webrev.00/ The change is simply to move those 3 resources under sun.text.resources.ext package so that it won't cause the split package issue. Naoto