Preliminary request for review: 7025066 Build system changes to support SE Embedded integration

[Dr Andrew John Hughes]

On 09:39 Mon 07 Mar     , Kelly O'Hair wrote:

General comments:

* Could this not be broken up into smaller changesets to make it easier to review and catch regressions?
* There seem to be some whitespace changes that shouldn't be there.

e.g.

-	sane-msvcrt_path \
+	sane-msvcrt_path \

> Just to clarify for people, BUILD_CLIENT_ONLY refers to building the client VM only.
> Some of these variables should be documented in the top level README-builds.html file, but that
> can be done under a separate CR if necessary.
> 

What happens if there is no client VM e.g. x86_64?
I think if this patch introduces it, the documentation should be part of this patch.

> The Library.gmk file seems like it is just a leftover debugging echo?
> 
> Looks like the Program.gmk file should be sharing the LD_RUNPATH_EXTRAS logic somehow, I originally
> was trying to isolate all the $ORIGIN option setting to one place, oh well, not your issue. You have actually
> cleanup some of these linker settings, so thanks for that.
> 
> The Release.gmk file needs  a major overhaul. :^(  I'm wondering if the various things being added here could
> have their own separate functionality flags, rather than JAVASE_EMBEDDED,  have COMPRESS_JARS=true/false,
> REDUCED_JRE=true/false, then have one place where JAVASE_EMBEDDED turns on what it wants?

Strongly agree with this.  It would make more sense to have these sort
of changes introduced by separate changesets and then have this change
set them for the JAVASE_EMBEDDED build.  In particular, COMPRESS_JARS
was something I had planned on fixing.  To Kelly's list, I'd add
BUILD_ALSA and LIBZIP_CAN_USE_MMAP (mentioned below).  This would make
this work far more generally useful than it is at present.

> Like the BUILD_CLIENT_ONLY setting. Just thoughts.
> But I'm ok with what you have, just cringing a little whenever the file Release.gmk gets more added to it. :^(
> 
> In Defs-control.gmk you have a line with just "error ...some message" not exactly sure what you are expecting here.
> You also use USE_ONLY_BOOTDIR_TOOLS?=true  and I have avoided ?=. The reason is that it assigns the
> variable if it hasn't been defined, not if it was defined to be empty or expands to empty.
> Between ?= and ifdef/ifndef, it gets pretty confusing what "defined" means, and I've been favoring using just
>    ifeq ($(USE_ONLY_BOOTDIR_TOOLS),)
> as the best way to ask if a variable will expand to nothing, which is usually the real question in most situations.
> I have only used ifeq($(origin X),defined) when a variable being explicitly defined to an empty value needed to be
> allowed, and this ?= effectively uses ifeq($(origin X),defined). A little nit picking, sorry.
> 
> In make/common/shared/Defs.gmk, you define HOST_CC and have another "error" line. I'd rather not
> have the check in this file, but we should just define HOST_CC if not defined, and have HOST_CC checked
> in Sanity.gmk in a special embedded sanity check where you verify all embedded requirements you might have.
> We should probably also add some of these embedded variables added to Sanity-Settings.gmk so we can see
> the settings when 'make sanity' is run.
> 
> Sanity.gmk changes completely turns off the compiler version check, if you really want to do this, perhaps
> the better approach is to put this sanity check in a ifdef REQUIRED_CC_VER and then have you  not define
> that variable in Defs-versions.gmk? If you are saying you want NO required version, then not defining
> the REQUIRED_* variables seems like a good idea.
> 
> In java/zip/Makefile maybe we need a LIBZIP_CAN_USE_MMAP functionality option here?
> Then embedded wouldn't even need to be mentioned here.
> 
> In Version.java.template You have added a "Java (TM)" line, I think that needs to go. You should add that
> in via the makefile somehow but I don't think it belongs in an OpenJDK build.
> 

Has this been tested on an OpenJDK build at all?  It also seems to create a fonts.dir file with fonts
that aren't part of OpenJDK.

> -kto
> 
> 
> On Mar 7, 2011, at 2:14 AM, David Holmes wrote:
> 
> > http://cr.openjdk.java.net/~dholmes/7025066/webrev/
> > 
> > The SE-Embedded product is a combination of open and closed sources. To allow SE-Embedded to be built from standard OpenJDK sources we need to apply a number of changes to the SE 7 build system:
> > 
> > - support for building the hotspot client compiler only (hotspot already supports this, this is the JDK side of things)
> > - support for doing cross-compilation (Linux only)
> > - minimal support for ARM/PPC architectures in the open code that currently only knows about x86 and sparc
> > - SE-Embedded specific build settings and targets (specifically the headful and headless reduced JRE images)
> > 
> > ---
> > 
> > These changes are obviously primarily for Oracle's benefit, but some aspects may be use of externally as well. The hope is that these changes won't have an adverse affect on any downstream OpenJDK builders, but until I get feedback on that I won't know.
> > 
> > Thanks,
> > David Holmes
> > SE Embedded Team
> > 
> 

-- 
Andrew :)

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

Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D  0698 0713 C3ED F586 2A37