RFR: JDK-8027963: Create unlimited policy jars.
bradford.wetmore at oracle.com
Tue Dec 3 02:52:05 UTC 2013
Given the name of your workspace /localhome/hg/jdk8-tl, I'm assuming
this will be going through the TL gate instead of build? Please do use
tl, as there will be some parallel changes to the RE scripts that I need
to make, and we won't be surprised when the changes hit the gate weeks
When do you think this will go in?
One minor thing which is probably not a problem, there's a new blank
line at the end of the new MANIFEST files probably created by SetupArchive.
>> 197/254/257/281: This webrev was probably generated before the
>> makefile reorg was done. jdk/make/javax/crypto ->
>> 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"
> Added it on the first occurrence. Ideally configure should complain even
> earlier though.
Yes, it could.
>> 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.
Ok. I personally liked the old style, but I'm ok with your change.
>> 284: JARS? Do you want to call it FILES now? ;)
> I agree the name is not right. Renamed it TARGETS.
>> 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?
>> UNLIMITED can be removed, for same reason.
> After the removal of the old build, these can now be removed yes, done.
Looks good. Thanks!
>> I'll update 8027266 with what remains to be done.
>> 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
>>> 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
>>>> 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
>>>> 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.
More information about the build-dev