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

Andrew John Hughes gnu_andrew at member.fsf.org
Fri Aug 21 10:42:46 PDT 2009 

2009/8/21 John Coomes <John.Coomes at sun.com>:
> Andrew John Hughes (gnu_andrew at member.fsf.org) wrote:
>> 2009/8/20 John Coomes <John.Coomes at sun.com>:
>> > 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).
>> >
>>
>> I had a feeling Windows would be the one to bite...
>>
>> > 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.
>> >
>>
>> Ok, in http://cr.openjdk.java.net/~andrew/ecj/03/webrev.03/ I've
>> reverted the addtion
>> of defs.make in favour of using MAKE_ARGS and renamed the variables
>> using underscores (which I prefer anyway).
>>
>> Where I am confused is how the MAKE_ARGS changes get through to the
>> Windows build if it doesn't parse defs.make.
>> makefiles/windows/defs.make seems to imply that the top-level one will
>> be read.
>>
>> >> 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.
>> >
>>
>> Yes, the other problem is this forces the same options to SA as well
>> which uses COMPILE.JAVAC.  To keep the current status quo (at least
>> for now), these need a different set of flags.
>>
>> >> 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.
>>
>> That kind of assumption is the thing that tends to come back and bite
>> you.  It's especially true because:
>>
>>   * The boot JDK is something determined on a per-user basis, so just
>> updating the system JDK may cause a failure that's hard to diagnose.
>>   * Failures in HotSpot javac builds are already hard to diagnose
>> because it doesn't print out the javac invocation.  It took me longer
>> to track this down than the equivalents in JDK and CORBA, and I've
>> been working with the OpenJDK build for about two years now.
>
> FWIW, you can get hotspot builds to be verbose with
>
>        gmake ... QUIETLY=
>
> (i.e., set QUIETLY to an empty value) and it should show all command
> invocations.

Ah thanks for that.  Will probably make that the default for me by
adding it to my build script.  There's a thin line between being too
verbose (e.g. the mass warnings ecj spits out which obscure real
problems) and being too quiet that it becomes hard to debug an issue.
I think some of the uses of QUIETLY go a little too far towards the
latter.

>
>> Yes, ecj is the only one that fails.  But we now have at least two
>> JDKs that will produce different results: OpenJDK6 (which defaults to
>> 6) and OpenJDK7 (which defaults to 7).  There are even more if you
>> include the proprietary JDKs and JDK 1.5 which I assume can also build
>> OpenJDK at a push.
>> ...
>
> Different, but still correct.  I'm resisting a default because it will
> get stale (they always do, SA is a good example).  And it's worked for
> *years* (6? 8?) without a problem.  But I won't reject the rest of it
> because there's a default.  Sigh.
>

Correct if you regard having a mix of classfile versions in the
finishing JDK as correct.  We do already get that with SA but that's a
bug that needs to be fixed in the long run.

I understand where you're coming from with regards to stale defaults
and, in general, I agree with you.  But not specifying these doesn't
remove the default, it just obscures it because the default is now
implicit and will vary dependent on boot environment.

You can see it working for 6-8 years as both a positive and a
negative; the variance in boot JDK has clearly been quite small in
this time period as no-one has tried building with a compiler that has
a default below the minimum required to compile the code.  This is
reinforced by the fact that, while the classfile version is on its 8th
revision, I can think of only three source breakages: inner classes
(1.1), assert (1.4) and generics/enums/annotations (1.5).  And I would
imagine the use of assert is fairly low.

Aside from these general issues, from my perspective, we're going to
have this set somewhere for IcedTea builds as it's causing build
breakage.  I'd prefer that OpenJDK and IcedTea builds had as much in
common in possible.  Having it in OpenJDK itself also avoids this
coming up when another user runs across it; this will undoubtedly
happen as the next 6-8 years with OpenJDK as a Free Software project
are going to see a lot more variance in the people building the code
than in the last 6-8 years where it was proprietary and thus limited
to certain companies and academia.

>> Finally, switching SA to use the defaults would be a visible change.
>
> Dan mentioned the SA values in passing; my impression was he thought
> they should be updated or removed.  I don't work with the SA, but I
> suspect the settings are relics from using a boot jdk that had 1.3 as
> a default when SA needed 1.4 features.  Anyone recall?
>
> Even if for some odd reason SA source should be limited to using 1.4
> features, if you update JAVAC_COMPILE to include JAVAC_FLAGS (or
> whatever it is), and the SA makefile also includes -source/-target
> options on the command-line, the latter will override the ones in
> JAVAC_FLAGS (the SA command line may need to include both -source and
> -target).  At least try it, I think it'll be a simpler change.
>
> I submitted your latest patch to the build queue for test builds;
> hopefully it will send you mail when it's finished (but I've never
> tried including a non-Sun email address before).
>

Thanks.  I haven't received anything yet, though it could have been
lost in the deluge of email from the build promotion.  Any idea what
the subject of such a mail would be?

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


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

More information about the hotspot-dev mailing list