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

[David Holmes]

On 19/09/2012 3:17 AM, Joe Provino wrote:
> On 9/18/2012 2:44 AM, David Holmes wrote:
>> On 18/09/2012 10:30 AM, Joe Provino wrote:
>>> Sorry, I forgot to provide a link to the new webrev:
>>>
>>> http://cr.openjdk.java.net/~jprovino/7189254/webrev.04
>>
>> The webrev is missing excludeSrc.gmk and the new minimal1.make
>> files.(perhaps they were not "hg add"ed?)
>>
>> I applied your patch to a fresh checkout of hotspot-rt that was
>> situated in a jdk8 forest. Initially I hadn't noticed that
>> minimal1.make was missing which was a good in a way because it
>> highlighted a logic error in make/Makefile in generic_buildminimal1:
>>
>> + @if [ -r $(GAMMADIR)/make/$(OSNAME)/makefiles/minimal1.make ] ; then \
>> + $(CD) $(OUTPUTDIR); \
>> + $(MAKE) -f $(ABS_OS_MAKEFILE) $(MAKE_ARGS) $(VM_TARGET) ; \
>> + else \
>> + $(ECHO) "No $(VM_TARGET) for $(OSNAME)
>> ARCH_DATA_MODEL=$(ARCH_DATA_MODEL)" ; \
>> + fi
>>
>> If minimal1.make is missing this needs to be a fatal error, because
>> the build will fail later anyway because you have added minimal1
>> related targets to the EXPORT_LIST.
>
> minimal1.make shouldn't be missing. Do I need the @if or can I just get
> rid of it?

Just get rid of the if, minimal1.make should be there. If it isn't then 
the build will fail later when/if you try to use it. Platforms that 
don't support minimal will have already been rejected - right?

David

> joe
>
>>
>> FYI I was unable to complete a full build due to build machine issues
>> unrelated to this, but I could see minimal get built ok and gamma ran.
>>
>>>> David, I made all of the changes you wanted (listed in the link below)
>>>> except for the rule to make optimizedminimal1. optimizedminimal1 would
>>>> be the same as productminimal1.
>>
>> Thanks. They all seem to be addressed. One minor modification. In
>> make/defs.make can you move:
>>
>> 102 ifeq ($(USER),)
>> 103 USER=$(USERNAME)
>> 104 endif
>>
>> back up under
>>
>> 32 # Directory paths and user name
>> 33 # Unless GAMMADIR is set on the command line, search upward from
>> 34 # the current directory for a parent directory containing
>> "src/share/vm".
>> 35 # If that fails, look for $GAMMADIR in the environment.
>> 36 # When the tree of subdirs is built, this setting is stored in each
>> flags.make.
>> 37 GAMMADIR := $(shell until ([ -d dev ]&&echo
>> $${GAMMADIR:-/GAMMADIR/}) || ([ -d src/share/vm ]&&pwd); do cd ..; done)
>> 38 HS_SRC_DIR=$(GAMMADIR)/src
>> 39 HS_MAKE_DIR=$(GAMMADIR)/make
>> 40 HS_BUILD_DIR=$(GAMMADIR)/build
>>
>> so that the comment at #32 remains accurate. No need to regenerate
>> webrev just for that.
>>
>> Looking at the source changes there is an inconsistency with CDS
>> handling (as per my direct email re the JPRT crash). arguments.cpp is
>> guarding the CDS code using !INCLUDE_SERVICES not !INCLUDE_CDS:
>>
>> 2427 #elif !INCLUDE_SERVICES
>> 2428 vm_exit_during_initialization(
>> 2429 "Dumping a shared archive is not supported int this VM.", NULL);
>> 2430 #else
>>
>> and
>>
>> 3171 #if !INCLUDE_SERVICES
>> 3172 no_shared_spaces();
>> 3173 #endif // INCLUDE_SERVICES
>>
>> Also note there is a minor type on #2429: int -> in
>>
>> Thanks,
>> David
>>
>>
>>>>
>>>> joe
>>>>
>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2012-August/006406.html
>>>>
>>>>
>>>