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

[John Coomes]

Andrew John Hughes (gnu_andrew at member.fsf.org) wrote:
> 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/

I submitted your patch to the build queue; it will take several hours,
there's one job ahead of it.

After looking at it again, the include of make/defs.make in
make/windows/makefiles/rules.make will most likely fail to build.  The
latter is a windows nmake file, so the gnu-specific syntax will be
rejected and there are other unixisms in there (e.g., shell until).

Instead, try updating MAKE_ARGS in make/defs.make, I believe those are
exported to sub-makes.  Since it has to be portable, I believe you'll
have to use '_' instead of '.' in the macro name.

> 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.

Take a look, it's simply selecting which path to use.  After the def
of COMPILE.JAVAC, a simple

	COMPILE.JAVAC += $(BOOTSTRAP_JAVAC.FLAGS)

should do it.  Windows will have to be different, though.

> 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.

The jdk, langtools, etc., repos are much more sensitive to this
setting than hotspot, which uses it only for makedeps and jvmti header
generation.  It's non-critical, and I think it's safe to assume that
the default class file version produced by a boot jdk can also be run
by that same boot jdk.

So far, ecj is the only known boot env needing this set explicitly.

-John

> >> 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