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

[Joe Provino]

I've made all of the changes David mentioned.

The latest webrev is here:

http://cr.openjdk.java.net/~jprovino/7189254/webrev.07 
<http://cr.openjdk.java.net/%7Ejprovino/7189254/webrev.07>

thanks.

joe

On 9/18/2012 9:35 PM, David Holmes wrote:
> 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 
>>>>>
>>>>>
>>>>>
>>>>