Hi Goetz,

I will sponsor this.

Thanks,
David

On 1/09/2017 12:49 AM, Lindenmaier, Goetz wrote:
Hi,

thanks for reviewing everybody!
Yes, works fine without that assignment. New webrev:
http://cr.openjdk.java.net/~goetz/wr17/8186978-disableCDS/webrev.02/

Could someone please sponsor? I think autogen.sh needs to be run
before submitting.

Best regards,
   Goetz.

-----Original Message-----
From: Magnus Ihse Bursie [mailto:magnus.ihse.bur...@oracle.com]
Sent: Thursday, August 31, 2017 3:35 PM
To: David Holmes <david.hol...@oracle.com>; Lindenmaier, Goetz
<goetz.lindenma...@sap.com>; hotspot-runtime-...@openjdk.java.net;
build-dev (build-dev@openjdk.java.net) <build-dev@openjdk.java.net>
Subject: Re: RFR(M): 8186978: Introduce configure argument enable-cds



On 2017-08-31 14:47, David Holmes wrote:
Hi Goetz,

On 31/08/2017 10:29 PM, Lindenmaier, Goetz wrote:
Hi,

Tests for class data sharing (cds) are enabled if @requires vm.cds is
true.
The property vm.cds depends on the preprocessor macro ENABLE_CDS.
... but you mean INCLUDE_CDS. :-)

This can not yet be switched by configure. It's only disabled
automatically
for the minimal build.

This change introduces enable-cds with default true, which only takes
effect
in the non-minimal build. If disabled, generate-classlist is
disabled, too.

Please review this change. I please need a sponsor.
http://cr.openjdk.java.net/~goetz/wr17/8186978-
disableCDS/webrev.01/index.html


I'll let the build guys comment in detail, but the structure for this
doesn't quite look right to me. I don't understand why you have in
spec.gmk.in:

+ ENABLE_CDS:=@ENABLE_CDS@

when in the hotspot build CDS is controlled via the feature setting:

ifneq ($(call check-jvm-feature, cds), true)

which you are already handling. ??

Agree, the ENABLE_CDS variable is only used internally in the configure
script and need not/should not be exported in spec.gmk.in. As David
says, the test ($(call check-jvm-feature, cds), true) is enough to
determine if to send the -DINCLUDE_CDS to the compiler.

Just remove the changes to spec.gmk.in, and I'm ok with the patch.

/Magnus



Thanks,
David


Best regards,
    Goetz.


Reply via email to