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 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 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)
