[PATCH FOR REVIEW]: Make source/target options explicit for MakeDeps and jvmti

[Andrew John Hughes]

2009/8/18 John Coomes <John.Coomes at sun.com>:
> Daniel D. Daugherty (Daniel.Daugherty at Sun.COM) wrote:
>> Andrew,
>>
>> A couple of comments:
>>
>> - if you specify '-source 6', then '-target 6' is implied
>>   so you don't really need both
>> - I would prefer to see a top-level macro that defines the
>>   bootstrap tools javac option and then have the various
>>   Makefiles use that macro. Something in make/defs.make:
>>
>>       BOOTSTRAP_TOOLS.JAVAC.FLAGS="-source 6"
>>
>>   or something like that.
>
> +1 on the top-level macro.  In HotSpot, I would call it JAVAC_FLAGS or
> COMPILE_JAVAC_FLAGS, and add it to the COMPILE.JAVAC macro (spelled
> COMPILE_JAVAC on windows) so that every compilation uses it.  Then
> fewer places need to be touched.
>
> Also, please leave the default value empty.  On platforms that need
> it, it can be set on the command line or in the environment.
> Specifying a value requires periodic maintenance.  Yes, the period is
> long, but it's a pain when the build breaks because of some stale
> value in a makefile.
>

Ok, here's the changeset I was working on before I received your mail
and which I've since successfully tested with hotspot-rt:

http://cr.openjdk.java.net/~andrew/ecj/03/webrev.02/

This defines the variables in the top-level defs.make file.  The
versions are stored separately, in the same way as they are in the
JDK:

http://hg.openjdk.java.net/jdk7/build/jdk/rev/80368890a2a0

SA is still done separately as it's currently 1.4.  The default for others is 6.
The rest of the patch ensures this makefile is picked up and its
values used for compilation across all three platforms.

This is added to the invocations rather than COMPILE.JAVAC.  As you
imply in your mail, this value varies between platforms and even
within the same platform.  Just the GNU/Linux port itself defines it
in about four ways.

I don't agree with leaving the value blank.  The issue is relevant on
all platforms, as leaving it blank makes the build open to whatever
the default value is of the bootstrap JDK.  For instance, this just
changed to 7 with OpenJDK, so current builds will produce 1.7 bytecode
while builds using an older OpenJDK7 or OpenJDK6 will use 6.  From
personal experience, it's much harder to debug a problem when you have
to take into account the boot JDK being used, which may vary, than
when you have an explicit value.

It seems much clearer to specify it explicitly in one common place for
all platforms, which can also be easily overridden from the command
line if needed.  This is the same as is now done for JDK, langtools,
jaxp and jaxws and will shortly be the case in corba.

>> Which repo are you targeting for this change?
>
> I'd suggest hotspot-rt.
>
> Note that all hotspot changes have to be built on all platforms before
> being pushed; we have an automated system called jprt that does builds
> and tests on the various platforms.  Unfortunately, it isn't available
> externally.  Sync up with the latest hotspot-rt and then contact me
> privately with a patch or your repo location and I'll run it through.
>

The patch from this webrev is against hotspot-rt so can be tested.

>> Yes, I see that SA uses '-source 1.4' in each platform makefile.
>> That should be fixed also, but that's another issue...
>
> Sigh.  Stale values in makefiles ...
>

Also fixed now by this patch.  At least 1.4 is in one common makefile
rather than occurring six times over three makefiles... :)

> -John
>
>> > Andrew John Hughes wrote:
>> > The makeDeps and jvmti bootstrap tools are built without explicit
>> > source and target options.
>> >
>> > This webrev:
>> >
>> > http://cr.openjdk.java.net/~andrew/ecj/03/webrev.01/
>> >
>> > sets them to 6 explicitly. The change has to be replicated three
>> > times, because for some reason, the javac invocations are in
>> > platform-specific makefiles.  I've tested the change successfully on
>> > GNU/Linux, where it allows ecj to be used as javac.  ecj defaults to
>> > <1.5 and thus otherwise fails to build.  I've assumed that the same
>> > changes are applicable for Solaris and Windows, and that the source
>> > and target options don't have to be written in Latin or something to
>> > work on these platforms.
>> >
>> > Is this ok to push?
>> >
>> > Thanks,
>> >
>
>



-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8