Re: RFR 7191662: JCE providers should be located via ServiceLoader
I skimmed it and looks okay to me. Thanks for separating this build work and avoid interfering the jimage refresh work. Mandy On Jun 15, 2015, at 4:58 PM, Valerie Peng valerie.p...@oracle.com wrote: It seems that the jimage refresh change may still take some time, so we will end up removing the makefile related changes and then deferring the ServiceLoader related changes to Jake workspace. Here is the latest webrev (Security source/test changes only, no more makefile changes) http://cr.openjdk.java.net/~valeriep/7191662/webrev.04/ Summary of changes from webrev.03: 1) switch back to provider class names for java.security file 2) separated out the java.policy change into its own as SQE has filed a bug for it 3) updated sun.security.jca.Providers class due to 1) 4) fixed sun.security.tools.jarsigner.Main to use the new Provider.configure() api 5) fixed a bug in sun.security.pkcs11.PKCS11Test regarding searching and configuring PKCS11 provider Thanks, Valerie On 6/8/2015 4:44 PM, Valerie Peng wrote: What is the bug id for the image refresh change? Just so I can keep a watch and hold my changes for the time being. My webrev has a new regression test which compares the list of providers found by ServiceLoader and Security.getProviders() call. So, if the META-INF/services/java.security.Provider file content is not correct, the new test would fail and it's a clear indication that ServiceLoader can't find one or more of the providers. Thanks, Valerie On 6/5/2015 10:43 PM, Mandy Chung wrote: On Jun 5, 2015, at 4:32 PM, Valerie Pengvalerie.p...@oracle.com wrote: I don't know image builder well enough to answer your question. The current implementation of the image builder sorts the modules by their name that are mapped to the same class loader. That explains why java.naming is the first one containing META-INF/services/java.security.Provider in your current list. There is no guarantee in what particular order a module is processed first. I don’t know if the jimage refresh work will change the ordering either. Since this is temporary, I’m okay if you want to depend on the image build implementation as long as you have a test to catch it and verify java.naming/META-INF/services/java.security.Provider file content. The existing security tests may also catch it but I think it’s not obvious to indicate that the security tests fail because of the issue of the merged service configuration file. Please also hold off until the image refresh change goes into jdk9/dev so that you can verify if your build change still works. If you want to push earlier, another alternative we discussed earlier is to separate the META-INF/services file and make change and java.security to keep using classname. That can be pushed that in a second phase with a proper image builder support. Mandy Based on my own experiment, it seems to pick up the one from java.naming when duplication occurs, so that's why I saved the merged result to there and named the Gensrc makefile with java.naming. The result build work fine. Does this explain what I am trying to do here? If you have better ways to get this done, I am certainly open to that idea. Thanks, Valerie On 6/5/2015 12:21 AM, Erik Joelsson wrote: Hello Valerie, The merging seems ok, but I thought there was non determinism in the image builder regarding which provider would get picked up. Is that resolved or do you really need to override all of those providers with your generated file in gensrc? I can assist in writing that makefile logic if needed. /Erik On 2015-06-04 23:58, Valerie Peng wrote: Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The
Re: RFR 7191662: JCE providers should be located via ServiceLoader
It seems that the jimage refresh change may still take some time, so we will end up removing the makefile related changes and then deferring the ServiceLoader related changes to Jake workspace. Here is the latest webrev (Security source/test changes only, no more makefile changes) http://cr.openjdk.java.net/~valeriep/7191662/webrev.04/ Summary of changes from webrev.03: 1) switch back to provider class names for java.security file 2) separated out the java.policy change into its own as SQE has filed a bug for it 3) updated sun.security.jca.Providers class due to 1) 4) fixed sun.security.tools.jarsigner.Main to use the new Provider.configure() api 5) fixed a bug in sun.security.pkcs11.PKCS11Test regarding searching and configuring PKCS11 provider Thanks, Valerie On 6/8/2015 4:44 PM, Valerie Peng wrote: What is the bug id for the image refresh change? Just so I can keep a watch and hold my changes for the time being. My webrev has a new regression test which compares the list of providers found by ServiceLoader and Security.getProviders() call. So, if the META-INF/services/java.security.Provider file content is not correct, the new test would fail and it's a clear indication that ServiceLoader can't find one or more of the providers. Thanks, Valerie On 6/5/2015 10:43 PM, Mandy Chung wrote: On Jun 5, 2015, at 4:32 PM, Valerie Pengvalerie.p...@oracle.com wrote: I don't know image builder well enough to answer your question. The current implementation of the image builder sorts the modules by their name that are mapped to the same class loader. That explains why java.naming is the first one containing META-INF/services/java.security.Provider in your current list. There is no guarantee in what particular order a module is processed first. I don’t know if the jimage refresh work will change the ordering either. Since this is temporary, I’m okay if you want to depend on the image build implementation as long as you have a test to catch it and verify java.naming/META-INF/services/java.security.Provider file content. The existing security tests may also catch it but I think it’s not obvious to indicate that the security tests fail because of the issue of the merged service configuration file. Please also hold off until the image refresh change goes into jdk9/dev so that you can verify if your build change still works. If you want to push earlier, another alternative we discussed earlier is to separate the META-INF/services file and make change and java.security to keep using classname. That can be pushed that in a second phase with a proper image builder support. Mandy Based on my own experiment, it seems to pick up the one from java.naming when duplication occurs, so that's why I saved the merged result to there and named the Gensrc makefile with java.naming. The result build work fine. Does this explain what I am trying to do here? If you have better ways to get this done, I am certainly open to that idea. Thanks, Valerie On 6/5/2015 12:21 AM, Erik Joelsson wrote: Hello Valerie, The merging seems ok, but I thought there was non determinism in the image builder regarding which provider would get picked up. Is that resolved or do you really need to override all of those providers with your generated file in gensrc? I can assist in writing that makefile logic if needed. /Erik On 2015-06-04 23:58, Valerie Peng wrote: Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the
Re: RFR 7191662: JCE providers should be located via ServiceLoader
What is the bug id for the image refresh change? Just so I can keep a watch and hold my changes for the time being. My webrev has a new regression test which compares the list of providers found by ServiceLoader and Security.getProviders() call. So, if the META-INF/services/java.security.Provider file content is not correct, the new test would fail and it's a clear indication that ServiceLoader can't find one or more of the providers. Thanks, Valerie On 6/5/2015 10:43 PM, Mandy Chung wrote: On Jun 5, 2015, at 4:32 PM, Valerie Pengvalerie.p...@oracle.com wrote: I don't know image builder well enough to answer your question. The current implementation of the image builder sorts the modules by their name that are mapped to the same class loader. That explains why java.naming is the first one containing META-INF/services/java.security.Provider in your current list. There is no guarantee in what particular order a module is processed first. I don’t know if the jimage refresh work will change the ordering either. Since this is temporary, I’m okay if you want to depend on the image build implementation as long as you have a test to catch it and verify java.naming/META-INF/services/java.security.Provider file content. The existing security tests may also catch it but I think it’s not obvious to indicate that the security tests fail because of the issue of the merged service configuration file. Please also hold off until the image refresh change goes into jdk9/dev so that you can verify if your build change still works. If you want to push earlier, another alternative we discussed earlier is to separate the META-INF/services file and make change and java.security to keep using classname. That can be pushed that in a second phase with a proper image builder support. Mandy Based on my own experiment, it seems to pick up the one from java.naming when duplication occurs, so that's why I saved the merged result to there and named the Gensrc makefile with java.naming. The result build work fine. Does this explain what I am trying to do here? If you have better ways to get this done, I am certainly open to that idea. Thanks, Valerie On 6/5/2015 12:21 AM, Erik Joelsson wrote: Hello Valerie, The merging seems ok, but I thought there was non determinism in the image builder regarding which provider would get picked up. Is that resolved or do you really need to override all of those providers with your generated file in gensrc? I can assist in writing that makefile logic if needed. /Erik On 2015-06-04 23:58, Valerie Peng wrote: Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie I'm not sure I followed completely here were this landed. Does this mean that there's currently no need for a build system code review on http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/, and that a new webrev will be posted instead? /Magnus . Mandy On
Re: RFR 7191662: JCE providers should be located via ServiceLoader
JDK-8066492 Mandy On 06/08/2015 04:44 PM, Valerie Peng wrote: What is the bug id for the image refresh change? Just so I can keep a watch and hold my changes for the time being. My webrev has a new regression test which compares the list of providers found by ServiceLoader and Security.getProviders() call. So, if the META-INF/services/java.security.Provider file content is not correct, the new test would fail and it's a clear indication that ServiceLoader can't find one or more of the providers. Thanks, Valerie On 6/5/2015 10:43 PM, Mandy Chung wrote: On Jun 5, 2015, at 4:32 PM, Valerie Pengvalerie.p...@oracle.com wrote: I don't know image builder well enough to answer your question. The current implementation of the image builder sorts the modules by their name that are mapped to the same class loader. That explains why java.naming is the first one containing META-INF/services/java.security.Provider in your current list. There is no guarantee in what particular order a module is processed first. I don’t know if the jimage refresh work will change the ordering either. Since this is temporary, I’m okay if you want to depend on the image build implementation as long as you have a test to catch it and verify java.naming/META-INF/services/java.security.Provider file content. The existing security tests may also catch it but I think it’s not obvious to indicate that the security tests fail because of the issue of the merged service configuration file. Please also hold off until the image refresh change goes into jdk9/dev so that you can verify if your build change still works. If you want to push earlier, another alternative we discussed earlier is to separate the META-INF/services file and make change and java.security to keep using classname. That can be pushed that in a second phase with a proper image builder support. Mandy Based on my own experiment, it seems to pick up the one from java.naming when duplication occurs, so that's why I saved the merged result to there and named the Gensrc makefile with java.naming. The result build work fine. Does this explain what I am trying to do here? If you have better ways to get this done, I am certainly open to that idea. Thanks, Valerie On 6/5/2015 12:21 AM, Erik Joelsson wrote: Hello Valerie, The merging seems ok, but I thought there was non determinism in the image builder regarding which provider would get picked up. Is that resolved or do you really need to override all of those providers with your generated file in gensrc? I can assist in writing that makefile logic if needed. /Erik On 2015-06-04 23:58, Valerie Peng wrote: Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie I'm not sure I followed completely here were this landed. Does this mean that there's currently no need for a build system code review on
Re: RFR 7191662: JCE providers should be located via ServiceLoader
Hello Valerie, The merging seems ok, but I thought there was non determinism in the image builder regarding which provider would get picked up. Is that resolved or do you really need to override all of those providers with your generated file in gensrc? I can assist in writing that makefile logic if needed. /Erik On 2015-06-04 23:58, Valerie Peng wrote: Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie I'm not sure I followed completely here were this landed. Does this mean that there's currently no need for a build system code review on http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/, and that a new webrev will be posted instead? /Magnus . Mandy On May 27, 2015, at 3:29 PM, Valerie Pengvalerie.p...@oracle.com wrote: Hi, build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/ This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. The rest of source code changes are reviewed by my team already. Thanks, Valerie (Java Security Team)
Re: RFR 7191662: JCE providers should be located via ServiceLoader
On Jun 5, 2015, at 4:32 PM, Valerie Peng valerie.p...@oracle.com wrote: I don't know image builder well enough to answer your question. The current implementation of the image builder sorts the modules by their name that are mapped to the same class loader. That explains why java.naming is the first one containing META-INF/services/java.security.Provider in your current list. There is no guarantee in what particular order a module is processed first. I don’t know if the jimage refresh work will change the ordering either. Since this is temporary, I’m okay if you want to depend on the image build implementation as long as you have a test to catch it and verify java.naming/META-INF/services/java.security.Provider file content. The existing security tests may also catch it but I think it’s not obvious to indicate that the security tests fail because of the issue of the merged service configuration file. Please also hold off until the image refresh change goes into jdk9/dev so that you can verify if your build change still works. If you want to push earlier, another alternative we discussed earlier is to separate the META-INF/services file and make change and java.security to keep using classname. That can be pushed that in a second phase with a proper image builder support. Mandy Based on my own experiment, it seems to pick up the one from java.naming when duplication occurs, so that's why I saved the merged result to there and named the Gensrc makefile with java.naming. The result build work fine. Does this explain what I am trying to do here? If you have better ways to get this done, I am certainly open to that idea. Thanks, Valerie On 6/5/2015 12:21 AM, Erik Joelsson wrote: Hello Valerie, The merging seems ok, but I thought there was non determinism in the image builder regarding which provider would get picked up. Is that resolved or do you really need to override all of those providers with your generated file in gensrc? I can assist in writing that makefile logic if needed. /Erik On 2015-06-04 23:58, Valerie Peng wrote: Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie I'm not sure I followed completely here were this landed. Does this mean that there's currently no need for a build system code review on http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/, and that a new webrev will be posted instead? /Magnus . Mandy On May 27, 2015, at 3:29 PM, Valerie Pengvalerie.p...@oracle.com wrote: Hi, build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/ This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image
Re: RFR 7191662: JCE providers should be located via ServiceLoader
Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie I'm not sure I followed completely here were this landed. Does this mean that there's currently no need for a build system code review on http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/, and that a new webrev will be posted instead? /Magnus . Mandy On May 27, 2015, at 3:29 PM, Valerie Pengvalerie.p...@oracle.com wrote: Hi, build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/ This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. The rest of source code changes are reviewed by my team already. Thanks, Valerie (Java Security Team)
Re: RFR 7191662: JCE providers should be located via ServiceLoader
Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie I'm not sure I followed completely here were this landed. Does this mean that there's currently no need for a build system code review on http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/, and that a new webrev will be posted instead? /Magnus . Mandy On May 27, 2015, at 3:29 PM, Valerie Pengvalerie.p...@oracle.com wrote: Hi, build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/ This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. The rest of source code changes are reviewed by my team already. Thanks, Valerie (Java Security Team)
Re: RFR 7191662: JCE providers should be located via ServiceLoader
Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie . Mandy On May 27, 2015, at 3:29 PM, Valerie Pengvalerie.p...@oracle.com wrote: Hi, build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/ This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. The rest of source code changes are reviewed by my team already. Thanks, Valerie (Java Security Team)
Re: RFR 7191662: JCE providers should be located via ServiceLoader
On 27/05/2015 23:42, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev. I think that's the best approach and this will also allow the temporary image build tool to be updated to merge the configuration files. -Alan
Re: RFR 7191662: JCE providers should be located via ServiceLoader
Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev. Mandy On May 27, 2015, at 3:29 PM, Valerie Peng valerie.p...@oracle.com wrote: Hi, build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/ This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. The rest of source code changes are reviewed by my team already. Thanks, Valerie (Java Security Team)
RFR 7191662: JCE providers should be located via ServiceLoader
Hi, build experts, Can you please review the make file related change, i.e. the new file || *make/gensrc/Gensrc-java.naming.gmk*, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/ This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. The rest of source code changes are reviewed by my team already. Thanks, Valerie (Java Security Team)
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
On 2015-05-22 18:53, Mandy Chung wrote: On 05/22/2015 08:09 AM, Alan Bateman wrote: On 22/05/2015 13:55, Chris Hegarty wrote: : I think it could be done either way. Valerie - have you considered not pushing the services configuration files with this change? With the change then the java.security configuration is still class names, not provider names, so the fallback should just work. This is what we've done in a few other areas (like JNDI for example). I wasn't aware of the other areas that move to service provider but remain being loaded with the fallback Class.forName. I would prefer java.security should convert to use the provider names as an example and also exercise the code path using service providers. If this causes much work to workaround it temporarily, I won't object the security providers are not truly service providers (no META-INF/services and java.security lists class name instead) Another option to workaround this: we only need to merge the service config files for generating the image. Can we have do the concatenation of jdk/modules/*/META-INF/services file and output to supports/image_gensrc before the images target and have the image builder to exclude all jdk/modules/*/META-INF/services files and take the supports/image_gensrc instead? This will remove the process-provider logic from Gensrc-*.gmk files. Would this be a better alternative? Maybe, I'm not sure. I still think solving this in java code in the ImageBuilder is the right thing to do. /Erik
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
On 25/05/15 09:42, Erik Joelsson wrote: On 2015-05-22 18:53, Mandy Chung wrote: On 05/22/2015 08:09 AM, Alan Bateman wrote: On 22/05/2015 13:55, Chris Hegarty wrote: : I think it could be done either way. Valerie - have you considered not pushing the services configuration files with this change? With the change then the java.security configuration is still class names, not provider names, so the fallback should just work. This is what we've done in a few other areas (like JNDI for example). I wasn't aware of the other areas that move to service provider but remain being loaded with the fallback Class.forName. URL protocol handlers, and JDNI initial context, fall back to Class.forName for built-in handlers/implementations, and do not expose the implementation as services. I would prefer java.security should convert to use the provider names as an example and also exercise the code path using service I think URL protocol handlers works well as an example too. It is a minimal new service type, easy to understand. You can write your own Foo handler in a few lines of code. providers. If this causes much work to workaround it temporarily, I won't object the security providers are not truly service providers (no META-INF/services and java.security lists class name instead) Another option to workaround this: we only need to merge the service config files for generating the image. Can we have do the concatenation of jdk/modules/*/META-INF/services file and output to supports/image_gensrc before the images target and have the image builder to exclude all jdk/modules/*/META-INF/services files and take the supports/image_gensrc instead? This will remove the process-provider logic from Gensrc-*.gmk files. Would this be a better alternative? Maybe, I'm not sure. I still think solving this in java code in the ImageBuilder is the right thing to do. If it is agreed that these files are needed, then I can look at expanding the ImageBuilder to do concatenate them. -Chris. /Erik
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
On 25/05/2015 09:53, Chris Hegarty wrote: If it is agreed that these files are needed, then I can look at expanding the ImageBuilder to do concatenate them. I agree with Mandy's point that java.security should be change to list the provider name rather than the class name. If that happens then it means that the service configuration files will be required. I don't have a strong view on whether the concatenation is done via make files or the image builder as it's all just temporary and will go away once resources are keyed by a module. One thing about rev'ing the image builder is that we should probably let the jimage refresh get into jdk9/dev first. I don't think we want to delay that due to merge conflicts. -Alan
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
On May 25, 2015, at 3:00 AM, Alan Bateman alan.bate...@oracle.com wrote: On 25/05/2015 09:53, Chris Hegarty wrote: If it is agreed that these files are needed, then I can look at expanding the ImageBuilder to do concatenate them. I agree with Mandy's point that java.security should be change to list the provider name rather than the class name. If that happens then it means that the service configuration files will be required. I don't have a strong view on whether the concatenation is done via make files or the image builder as it's all just temporary and will go away once resources are keyed by a module. One thing about rev'ing the image builder is that we should probably let the jimage refresh get into jdk9/dev first. I don't think we want to delay that due to merge conflicts. Right this is all temporary. One benefit of having the concatenation done by image builder will get the makefile cleaned up and get ready for the resources keyed for a module as long as the work required is small. Good point about image refresh and any image builder change should come after jimage refresh to avoid causing any merge conflict. Depending on when the image refresh and this security provider change are ready to push to jdk9/dev, I can work with Valerie to determine whether to wait or phase it. thanks Mandy
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
On 2015-05-22 02:46, Mandy Chung wrote: I’m including build-dev and we need to ask for Erik and Magnus advice what’s the best way to work around this. Erik, Magnus, Security providers now become service providers. They are provided from 11 different modules, 3 of them are os-specific. The current image builder doesn’t merge duplicate resources and we currently work around it in the build doing the merge as it’s temporary until the module system is moving further. Gensrc-jdk.jdi.gmk is the example. Valerie has a patch attempting to concatenate them and it turns out that it might not be straight forward I have thought. http://cr.openjdk.java.net/~valeriep/7191662/webrev.00/make/gensrc/Gensrc-java.naming.gmk.html As we can’t say which one image builder will pick, it needs to copy gensrc (merged version) to all modules/$MODULE/META-INF/services/java.security.Provider config files. I wonder if it’s quicker to hack ImageBuilder to special case the service config file and merge them. Any thought? I could certainly make this work in the makefile, but it is getting a bit tedious to keep these hacks going there. How hard would it be to change the ImageBuilder, which is just a temporary solution anyway, to identify and merge all service provider files with the same name that it finds? With that solution, the makefiles do not need to change as much when the module system is introduced and taking care of this properly. /Erik On May 21, 2015, at 3:03 PM, Valerie Peng valerie.p...@oracle.com wrote: Mandy, Please find comments in line. On 5/20/2015 10:39 PM, Mandy Chung wrote: A quick comment on the META-INF/services config files and the makefile. Merging the service config files is temporary until the module system is moving further along. src/jdk.crypto.ucrypto/solaris/classes/META-INF/services/java.security.Provider.html and other os-specific service configuration: 1 #[solaris]com.oracle.security.ucrypto.UcryptoProvider - why is this commented out? Does the makefile uncomment it? It should be simple concatenation with In an example that I found through another makefile, it would uncomment the entry start with #[OS] (and process/remove this prefix) when the OS matches. We need OS-specific file processing when concatenate these files. Ah… that’s from Gensrc-jdk.jdi.gmk. The service config file contains a mixture of cross-neutral and os-specific providers. That’s not the example to copy. define process-provider $(MKDIR) -p $(@D) $(CAT) $^ $@ endef What decides if it's appropriate or not? These are not just crypto providers that we are defining here, but all classes which extend from java.security.Provider. I recall using jdk.crypto.ec as the gensrc module as you suggested initially. But when testing it, it doesn't seem to work as expected. I ended up using java.naming as that's the one ended up in the final image instead of the concatenated one under jdk.crypto.ec. Could there be some alphabetic ordering when processing/building these modules? Well, since this is really a hack and only temporary, does it really matter whether it's under java.naming or jdk.crypto.ec? Both contains providers for the java.security provider list. The key thing is that the resulting image works Mandy
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
On 22/05/15 07:58, Erik Joelsson wrote: On 2015-05-22 02:46, Mandy Chung wrote: I’m including build-dev and we need to ask for Erik and Magnus advice what’s the best way to work around this. Erik, Magnus, Security providers now become service providers. They are provided from 11 different modules, 3 of them are os-specific. The current image builder doesn’t merge duplicate resources and we currently work around it in the build doing the merge as it’s temporary until the module system is moving further. Gensrc-jdk.jdi.gmk is the example. Valerie has a patch attempting to concatenate them and it turns out that it might not be straight forward I have thought. http://cr.openjdk.java.net/~valeriep/7191662/webrev.00/make/gensrc/Gensrc-java.naming.gmk.html As we can’t say which one image builder will pick, it needs to copy gensrc (merged version) to all modules/$MODULE/META-INF/services/java.security.Provider config files. I wonder if it’s quicker to hack ImageBuilder to special case the service config file and merge them. Any thought? I could certainly make this work in the makefile, but it is getting a bit tedious to keep these hacks going there. How hard would it be to change the ImageBuilder, which is just a temporary solution anyway, to identify and merge all service provider files with the same name that it finds? With that solution, the makefiles do not need to change as much when the module system is introduced and taking care of this properly. I think it could be done either way. -Chris. /Erik On May 21, 2015, at 3:03 PM, Valerie Peng valerie.p...@oracle.com wrote: Mandy, Please find comments in line. On 5/20/2015 10:39 PM, Mandy Chung wrote: A quick comment on the META-INF/services config files and the makefile. Merging the service config files is temporary until the module system is moving further along. src/jdk.crypto.ucrypto/solaris/classes/META-INF/services/java.security.Provider.html and other os-specific service configuration: 1 #[solaris]com.oracle.security.ucrypto.UcryptoProvider - why is this commented out? Does the makefile uncomment it? It should be simple concatenation with In an example that I found through another makefile, it would uncomment the entry start with #[OS] (and process/remove this prefix) when the OS matches. We need OS-specific file processing when concatenate these files. Ah… that’s from Gensrc-jdk.jdi.gmk. The service config file contains a mixture of cross-neutral and os-specific providers. That’s not the example to copy. define process-provider $(MKDIR) -p $(@D) $(CAT) $^ $@ endef What decides if it's appropriate or not? These are not just crypto providers that we are defining here, but all classes which extend from java.security.Provider. I recall using jdk.crypto.ec as the gensrc module as you suggested initially. But when testing it, it doesn't seem to work as expected. I ended up using java.naming as that's the one ended up in the final image instead of the concatenated one under jdk.crypto.ec. Could there be some alphabetic ordering when processing/building these modules? Well, since this is really a hack and only temporary, does it really matter whether it's under java.naming or jdk.crypto.ec? Both contains providers for the java.security provider list. The key thing is that the resulting image works Mandy
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
On 22/05/2015 13:55, Chris Hegarty wrote: : I think it could be done either way. Valerie - have you considered not pushing the services configuration files with this change? With the change then the java.security configuration is still class names, not provider names, so the fallback should just work. This is what we've done in a few other areas (like JNDI for example). -Alan
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
On 05/22/2015 08:09 AM, Alan Bateman wrote: On 22/05/2015 13:55, Chris Hegarty wrote: : I think it could be done either way. Valerie - have you considered not pushing the services configuration files with this change? With the change then the java.security configuration is still class names, not provider names, so the fallback should just work. This is what we've done in a few other areas (like JNDI for example). I wasn't aware of the other areas that move to service provider but remain being loaded with the fallback Class.forName. I would prefer java.security should convert to use the provider names as an example and also exercise the code path using service providers. If this causes much work to workaround it temporarily, I won't object the security providers are not truly service providers (no META-INF/services and java.security lists class name instead) Another option to workaround this: we only need to merge the service config files for generating the image. Can we have do the concatenation of jdk/modules/*/META-INF/services file and output to supports/image_gensrc before the images target and have the image builder to exclude all jdk/modules/*/META-INF/services files and take the supports/image_gensrc instead? This will remove the process-provider logic from Gensrc-*.gmk files. Would this be a better alternative? Mandy
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
I’m including build-dev and we need to ask for Erik and Magnus advice what’s the best way to work around this. Erik, Magnus, Security providers now become service providers. They are provided from 11 different modules, 3 of them are os-specific. The current image builder doesn’t merge duplicate resources and we currently work around it in the build doing the merge as it’s temporary until the module system is moving further. Gensrc-jdk.jdi.gmk is the example. Valerie has a patch attempting to concatenate them and it turns out that it might not be straight forward I have thought. http://cr.openjdk.java.net/~valeriep/7191662/webrev.00/make/gensrc/Gensrc-java.naming.gmk.html As we can’t say which one image builder will pick, it needs to copy gensrc (merged version) to all modules/$MODULE/META-INF/services/java.security.Provider config files. I wonder if it’s quicker to hack ImageBuilder to special case the service config file and merge them. Any thought? On May 21, 2015, at 3:03 PM, Valerie Peng valerie.p...@oracle.com wrote: Mandy, Please find comments in line. On 5/20/2015 10:39 PM, Mandy Chung wrote: A quick comment on the META-INF/services config files and the makefile. Merging the service config files is temporary until the module system is moving further along. src/jdk.crypto.ucrypto/solaris/classes/META-INF/services/java.security.Provider.html and other os-specific service configuration: 1 #[solaris]com.oracle.security.ucrypto.UcryptoProvider - why is this commented out? Does the makefile uncomment it? It should be simple concatenation with In an example that I found through another makefile, it would uncomment the entry start with #[OS] (and process/remove this prefix) when the OS matches. We need OS-specific file processing when concatenate these files. Ah… that’s from Gensrc-jdk.jdi.gmk. The service config file contains a mixture of cross-neutral and os-specific providers. That’s not the example to copy. define process-provider $(MKDIR) -p $(@D) $(CAT) $^ $@ endef What decides if it's appropriate or not? These are not just crypto providers that we are defining here, but all classes which extend from java.security.Provider. I recall using jdk.crypto.ec as the gensrc module as you suggested initially. But when testing it, it doesn't seem to work as expected. I ended up using java.naming as that's the one ended up in the final image instead of the concatenated one under jdk.crypto.ec. Could there be some alphabetic ordering when processing/building these modules? Well, since this is really a hack and only temporary, does it really matter whether it's under java.naming or jdk.crypto.ec? Both contains providers for the java.security provider list. The key thing is that the resulting image works Mandy
