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

[David Holmes]

Thanks Joe. Thumbs up from me.

For anyone else looking at this the latest changes concerning 
TEST_IN_BUILD are a pre-merge of the following changeset that is coming 
through hotspot-rt.

http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/989cf02ca531

The hotspot-emb gatekeeper will have to merge these.

David

On 22/09/2012 1:35 AM, Joe Provino wrote:
> 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
>>>>>>
>>>>>>
>>>>>>
>>>>>