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
