Request for Review: Conditionalize source so that functionality can be easily included or excluded

[David Holmes]

Hi Joe,

This is a review of the build changes only (so far).

On 30/08/2012 5:05 AM, Joe Provino wrote:
> CR7189361: Conditionalize source so that functionality can be easily
> included or excluded
> CR7189254: Change makefiles for more flexibility to override defaults
>
> See JEP-148 for details of the changes.

Expanding a little ... the old KERNEL build which used to strip various 
things out of the source has been reworked so that each component is 
identified distinctly. These components can then be included or excluded 
- potentially individually, but for now it is expected to be all-in or 
all-out. The exclusion mechanism operates at two levels:
- parts of files are guarded by ifdef INCLUDE_XXX
- entire files are excluded from the build using excludeSrc.make

Anyone working on one of these "optional" components needs to be aware 
of how to maintain that optionality.

Note that the old kernel build has not been eradicated from the build 
system - that is future clean up work. But it no longer gets used at all 
(and probably has not worked for some time).

This "minimal" VM is currently only supported on Linux and BSD. Some of 
the build changes will be seen to apply to Solaris/Windows as well, but 
that was for simplicity when copying and them modifying chunks of the 
build logic.

> The webrev is here:
>
> http://cr.openjdk.java.net/~jprovino/7189254/webrev.01.

make/Makefile:

   95 COMMON_VM_PRODUCT_TARGETS=product product1 productminimal1 docs 
export_product
   96 COMMON_VM_FASTDEBUG_TARGETS=fastdebug fastdebug1 fastdebugminimal1 
docs export_fastdebug
   97 COMMON_VM_DEBUG_TARGETS=jvmg jvmg1 jvmgminimal1 docs export_debug

This causes minimal to always be built by default - was that the intent? 
I note that this logic should be rewritten (as a future cleanup) to 
check the JVM_VARIANT* settings from the new build anyway.

  107 all_debug:     jvmg1 jvmgminimal1docs export_debug

Typo: need a space before docs

  120 all_optimized: optimized optimized1 docs export_optimized

optimizedminimal1 is not listed here - should it be?

271 else
  272         @$(ECHO) "No ($(VM_TARGET)) for $(OSNAME) 
ARCH_DATA_MODEL=$(ARCH_DATA_MODEL) for non-embedded."
  273 endif

The error message is wrong. You get here if generic_buildminimal1 is a 
target but JVM_VARIANT_MINIMAL is not true. I would think the error 
message would be: "Error: trying to build a minimal target but 
JVM_VARIANT_MINIMAL is not true".

346 ifeq ($(JVM_VARIANT_MINIMAL1), true)

Here you have used MINIMAL1 but elsewhere it is just MINIMAL. I think 
MINIMAL1 is probably the better choice. But obviously they have to be 
the same.

---

make/linux/Makefile (and bsd/Makefile)

345         cd $(OSNAME)_$(BUILDARCH)_minimal1/$(patsubst 
%minimal1,%,$@) && $(MAKE) $(MFLAGS) VARIANT=minimal1

Why do we set VARIANT when none of the other targets do?

---

make/linux/makefiles/gcc.make

  156 OPT_CFLAGS/SIZE += $(OPT_EXTRAS)
  157 OPT_CFLAGS/SPEED += $(OPT_EXTRAS)
  158 OPT_CFLAGS_DEFAULT ?= SPEED
  159
  ...
  163
  164 OPT_CFLAGS = $(OPT_CFLAGS/$(OPT_CFLAGS_DEFAULT))

Instead of lines 156 and 157, which pollute the size/speed flags, can't 
you just change 164 to:

OPT_CFLAGS = $(OPT_CFLAGS/$(OPT_CFLAGS_DEFAULT)) $(OPT_EXTRAS)

The same would apply to bsd/makefiles/gcc.make but the change there 
seems incomplete as you don't have the equivalent of line 164 from the 
linux version ie OPT_CFLAGS_DEFAULT never gets used to modify OPT_CFLAGS.

---

make/windows/makefiles/defs.make

  157     JVM_VARIANTS:=client,server,kernel
  158     JVM_VARIANT_CLIENT:=true
  159     JVM_VARIANT_SERVER:=true
  160     JVM_VARIANT_KERNEL:=false

If you are setting KERNEL to false then it also needs to be removed from 
JVM_VARIANTS. And you can just leave JVM_VARIANT_KERNEL undefined if it 
is not set to true.

---

Thanks,
David