RFR: JDK-8027963: Create unlimited policy jars.

Thu Nov 28 10:31:23 UTC 2013

Hello Brad,

New review posted:
http://cr.openjdk.java.net/~erikj/8027963/webrev.03/

On 2013-11-28 05:16, Bradford Wetmore wrote:
> 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.
>
Good to hear!
> 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.
>
Yes it was, the new one is generated from jdk8/tl which has the reorg.
> 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?
>
Yes, that will look tidier in the output dir. Moved it.
> 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
>
Added it on the first occurrence. Ideally configure should complain even 
earlier though.
> 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)
Done. Also changed all the other similar messages to the same format. 
The old copy messages have been annoying me for a while for not matching 
the style of other messages.
>
> 284:  JARS?  Do you want to call it FILES now?  ;)
I agree the name is not right. Renamed it TARGETS.
>
> 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.
>
After the removal of the old build, these can now be removed yes, done.

/Erik

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