On 15/03/2013 5:37 AM, Omair Majid wrote:
Hi,

Updated webrev at:
http://cr.openjdk.java.net/~omajid/webrevs/intree-ec/01/

I switched from DISABLE_INTREE_EC to ENABLE_INTREE_EC to avoid the
confusion with double negatives.

Looking just at the mechanics of this it looks fine to me. This needs to be coordinated with someone from the build team (which isn't me) so that we can keep the closed generated-configure.sh in sync.

Personally I wonder whether the existence check should be in the Makefile rather than done at configure time? I worry that we end up with too many make variables becoming configure variables as well.

David
-----

Note that because of the ifeq comparison, if you use the new build
system and just update the jdk tree, the ifeq ($ENABLE_INTREE_EC), yes)
comparison will fail (since ENABLE_INTREE_EC was not previously defined)
and EC will not be part of the build.

This problem wont happen if you update the root repo and re-run configure.

On 03/14/2013 06:03 AM, David Holmes wrote:
On 14/03/2013 3:12 PM, David Holmes wrote:
Note that this isn't changing any functionality simply exposing an
existing make variable at configure time.

Correction. I misunderstood what was being done here. This forcibly
set/clears the make variable based solely on the existence of a directory:

test -d "${SRC_ROOT}/jdk/src/share/native/sun/security/ec/impl"

Yes, it is forcibly set. But since this directory always exists in
OpenJDK 8, this should never evaluate to false.

This change is very useful for distributions, though, since they can
delete the directory when creating a source tarball for OpenJDK8, and
the ECC implementation will be automagically disabled from the build.

It doesn't expose a configure option for this. This may be perfectly
fine but the person who wrote the original TODO comment needs to verify
that.

Andrew Hughes wrote the original changeset in the old build system [1]
and I believe this is exactly what he wanted out of the changeset. I am
CC'ing Erik, who wrote the TODO [2] so he can clarify what he meant.

Thanks,
Omair

[1] http://hg.openjdk.java.net/jdk7/tl/jdk/rev/39c15c0a71f7
[2]
http://hg.openjdk.java.net/build-infra/jdk8/jdk/diff/1953cf522107/makefiles/CompileNativeLibraries.gmk

Reply via email to