RFR: JDK-8027963: Create unlimited policy jars.

Thu Nov 28 04:16:02 UTC 2013

Erik,

Thanks so much for handling much of the build work here.  I was planning 
on doing it, but you've got the background in the new build-infra so 
this probably much easier for you.  I've enjoyed learning new GnuMake 
techniques.

I've reviewed this pretty well, but didn't hit the previous 2 putbacks 
as hard as I wanted.

Now that I understand the black magic involved in the new makefiles 
(SetupArchive/SetupJavaCompiler/SetupJavaCompilation/etc), future 
reviews should be easier.

I haven't fully walked through the BUILD_CRYPTO cases, so I'll assume 
it's correct for this review.  I'm sure someone will holler if there's a 
problem.

CreateSecurityJars.gmk
======================

197/254/257/281:  This webrev was probably generated before the makefile 
reorg was done.  jdk/make/javax/crypto -> jdk/make/data/cryptopolicy/*

As a result, I didn't apply the bits and run a test build.  If you want 
to supply me either a built WS or an updated webrev, I can check things out.

198/251:  Since US_export_policy_jar.tmp is a tmp dir for policy, 
shouldn't this be in the jce/policy directory instead of the top level 
jce where the jars end up?

221/289:  Can you put in a error here for the case if an unlimited 
signed version is requested?  There is no prebuilt unlimited version, 
and people may not realize it.

     ifndef OPENJDK
       ifeq ($(UNLIMITED_CRYPTO), true)
         $(warning/error) "no prebuilt available"
       endif

221/290: Can you update these two copies of the policy file to say that 
you're copying the prebuilt policy files?

         $(ECHO) $(LOG_INFO) Copying $(@F)

284:  JARS?  Do you want to call it FILES now?  ;)

jdk/make/data/cryptopolicy/limited
==================================
LIMITED file can be removed since you are now adding this extra 
attribute via EXTRA_MANIFEST_ATTR.  Also, since you're explicitly 
listing the files to include, do you need the "SUFFIXES =" line?

jdk/make/data/cryptopolicy/unlimited
====================================
UNLIMITED can be removed, for same reason.

I'll update 8027266 with what remains to be done.

Brad

On 11/12/2013 2:24 AM, Erik Joelsson wrote:
> New webrev: http://cr.openjdk.java.net/~erikj/8027963/webrev.02/
>
> On 2013-11-08 13:08, Magnus Ihse Bursie wrote:
>> It took me some amount of reading and re-reading to get a grip on
>> this. Not only your changes but the original code as well. Some comments:
>>
> It's a bit of a mess yes.
>> I think the code for the US_export_policy could be clearer in that we
>> actually have only one file, the unlimited policy, and that we just
>> make an exact copy of this for the limited directory. For instance, I
>> got confused by the fact that we did not check the UNLIMITED_CRYPTO
>> variable. It might be more clear if we remove the limited version out
>> of the equation, and then as a separate step makes a copy. Or maybe
>> not. And maybe such a change should not be a part of this fix.
>>
> I tried to mimic the old build here where the limited and unlimited
> version are treated as different files all the way, except at initial
> creation.
>> However, there is a bug. For openjdk, the rule "
>> $(US_EXPORT_POLICY_JAR_DST): $(US_EXPORT_POLICY_JAR_UNSIGNED)" will
>> not work since US_EXPORT_POLICY_JAR_UNSIGNED is not defined anymore.
>> And the new equivalent, US_EXPORT_POLICY_JAR_UNLIMITED_UNSIGNED is
>> only defined inside an infeq.
>>
> Yes, I added the same conditional on UNLIMITED_CRYPTO as for
> local_policy. Again, this isn't really necessary since they are the same
> file.
>> The new log message " $(ECHO) Copying $(patsubst
>> $(OUTPUT_ROOT)/%,%,$@)" will be output on the default log level, but
>> the previous log message "$(ECHO) $(LOG_INFO) Copying $(@F)" was only
>> on the info level. I think this is more appropriate for information
>> about individual files.
> I added the message to make the output more consistent with "Creating
> ...jar", but you are probably right. I added the LOG_INFO macro.
>>
>> The changes in spec.gmk. in and SignJars.gmk look good, although the
>> README.txt file is only available if closed sources are present. I
>> suggest the whole SignJars file to be put in a "if not openjdk" block.
>> Long term, the file should probably move to closed sources.
> It actually already is, so this isn't an issue.
>>
>> For US_export as well as local policy: what is the reason of copying
>> the policy files to a temp directory before jar:ing them? Can't you
>> just point to the directory where the policy files reside? Aha, I see
>> now a comment: " TODO fix so that SetupArchive does not write files
>> into SRCS then we don't need this extra copying". Is that really still
>> the case? It sounds unlikely to me that SetupArchive modifies the
>> source path.
>>
> This is still the case yes. The SetupArchive macro assumes you are
> working on a classes directory in your build output. It stores
> information about which files are added from that particular source
> root. I took a quick look at fixing it, but decided against doing it in
> this bug.
>> For local_policy, when building unlimited crypto, the
>> default_US_export.policy seems to be included as well. On the other
>> hand, that seems to have been the case before your change as well.
>> However, this seems suspicious and I suspect that this is a bug.
>>
> It's not. Look at the dependencies declared for the SetupArchive macro
> calls. Those define which files will be in the temp directories.
>> For local policy, if building openjdk only and have BUILD_CRYPTO=no,
>> then install-file is going to fail, probably with a strange error
>> since the dependency variables are not defined. I know this is not a
>> supported combination but maybe we should fail gracefully.
>>
> This is already failing and while it might look better to fail nicely,
> configure will never put us in this situation.
>
> /Erik
>> /Magnus
>