I think that looks fine. -kto
On Jan 18, 2012, at 9:29 PM, David Holmes wrote: > FYI here is proposed webrev: > > http://cr.openjdk.java.net/~dholmes/7130909/webrev/ > > CR 7130909 was filed. > > David > > On 19/01/2012 1:30 PM, David Holmes wrote: >> Hi Kelly, >> >> On 19/01/2012 2:22 AM, Kelly O'Hair wrote: >>> >>> On Jan 17, 2012, at 7:17 PM, David Holmes wrote: >>> >>>> On 18/01/2012 7:07 AM, Kelly O'Hair wrote: >>>>> Generally, I have no objection to this change. >>>>> >>>>> But instead of setting it to NO_SUCH_PATH, I would not set it at >>>>> all, and avoid the $(shell) to see if it >>>>> exists completely. Just don't set CUSTOM_MAKE_DIR if it does not exist. >>>>> Then you don't need the HAS_CUSTOM_MAKE, it can just be ifdef >>>>> CUSTOM_MAKE_DIR >>>> >>>> But I/we want the default setting to occur in the build files and not >>>> require that it be set externally. My final suggestion gets rid of >>>> the HAS_CUSTOM_MAKE and the shell usage. Incorporating your >>>> suggestion it simplifies to: >>>> >>>> ifneq ($(OPENJDK),true) >>>> CUSTOM_MAKE_DIR = $(BUILDDIR)/closed >>>> endif >>> >>> Or something like: >>> >>> # Custom makefile additions >>> ifneq ($(OPENJDK),true) >>> CUSTOM_MAKE_DIR = $(BUILDDIR)/closed >>> # This directory should exist if OPENJDK is not "true" >>> ifeq ($(shell if [ ! -d $(CUSTOM_MAKE_DIR) ]; then echo "missing"; >>> fi),missing) >>> x:=$(error "ERROR: Not OPENJDK, and missing $(CUSTOM_MAKE_DIR)") >>> endif >>> endif >> >> Not sure we need to force it to be available. Seems this kind of thing >> belongs in a sanity test if anywhere, but personally I don't think it >> has to be enforced. >> >>>> >>>> in Defs.gmk and then: >>>> >>>> -include $(CUSTOM_MAKE_DIR)/<path-to-file.gmk> >>> >>> And: >>> >>> # Custom makefile additions >>> ifdef CUSTOM_MAKE_DIR >>> include $(CUSTOM_MAKE_DIR)/<path-to-file.gmk> >>> endif >> >> The -include is accounting not only for the directory not being present >> but also the actual file. Over time we may add these include hooks to >> support a number of different use-cases but that doesn't mean that >> anybody involved in any single use-case needs to provide every single >> custom file that might exist. >> >>>> >>>> as needed. Though I may get an error with CUSTOM_MAKE_DIR being >>>> undefined in which case I'll need to declare it for OpenJDK builds. >>> >>> Undefined is the same as being defined as empty in terms of expansion >>> $(var) >> >> Was unsure as I see examples where people ensure a variable is always >> defined in both an if and else clause - even if one is empty. >> >> Thanks, >> David >> ----- >> >>>> >>>>> Not sure I like the word CUSTOM, but that's just a name debate. >>>> >>>> It customizes the build logic. I preferred it to the "alt" name we >>>> use for hotspot as here it isn't an alternative but an addition. >>>> Anyway just a name game. >>> >>> Not a big deal, but kind of liked the piggyback idea. ;^) >>> >>> -kto >>> >>>> >>>> Thanks, >>>> David >>>> >>>>> Maybe ADDON or MODDING or MODS or oo oo PIGGYBACK? ;^) >>>>> http://3.bp.blogspot.com/-cHeD4qJSB44/TbMtSsAiY4I/AAAAAAAAA1c/gctQfVffu_o/s1600/PiggyBack.jpg >>>>> >>>>> >>>>> -kto >>>>> >>>>> On Jan 15, 2012, at 9:01 PM, David Holmes wrote: >>>>> >>>>>> Presently the build system contains some files, and directives >>>>>> involving those files, that supports the building of the Java SE >>>>>> Embedded product. What I would like to do is remove direct support >>>>>> for this and replace it with a more general mechanism that allows >>>>>> for custom make files to be included from an arbitrary location - >>>>>> which then allows for the removal of the embedded specific files. >>>>>> >>>>>> Initially I've modeled this on some other conditional mechanisms by >>>>>> defining the default location for non-OpenJDK builds, checking its >>>>>> existence and then using that as a guard for the individual files. >>>>>> >>>>>> http://cr.openjdk.java.net/~dholmes/custom-make/webrev/ >>>>>> >>>>>> ! ifneq ($(OPENJDK),true) >>>>>> ! CUSTOM_MAKE_DIR_REL=closed >>>>>> ! else >>>>>> ! CUSTOM_MAKE_DIR_REL=NO_SUCH_PATH >>>>>> ! endif >>>>>> ! CUSTOM_MAKE_DIR=$(BUILDDIR)/$(CUSTOM_MAKE_DIR_REL) >>>>>> >>>>>> + # Use this variable to guard inclusion of the custom files >>>>>> + HAS_CUSTOM_MAKE := $(shell if [ -d $(CUSTOM_MAKE_DIR) ]; then >>>>>> echo 1; else echo 0; fi) >>>>>> >>>>>> ... >>>>>> >>>>>> + ifeq ($(HAS_CUSTOM_MAKE),1) >>>>>> + include $(CUSTOM_MAKE_DIR)/Defs.gmk >>>>>> + endif >>>>>> >>>>>> Initially there are only two hooks for these custom files: >>>>>> >>>>>> 1. At the end common/Defs.gmk (as it gets included by all the >>>>>> Makefiles) >>>>>> 2. In the top-level JDK Makefile, including a custom Release.gmk >>>>>> >>>>>> Naturally these map to the existing uses of the *-embedded.gmk files. >>>>>> >>>>>> It then occurred to me that if someone wanted to add an additional >>>>>> hook somewhere else, that it might be best to check for the >>>>>> existence of the actual file rather than a root directory for all >>>>>> such files. That led me to consider a function >>>>>> custom_include(<filename>) to hide the existence logic. But I >>>>>> wasn't sure if a function could contain an include directive and >>>>>> upon checking on that I discovered a much simpler approach: the >>>>>> -include directive. This is like the include directive but does not >>>>>> trigger an error if the file does not exist. So we would simply >>>>>> have this in Defs.gmk: >>>>>> >>>>>> ifneq ($(OPENJDK),true) >>>>>> CUSTOM_MAKE_DIR_REL=closed >>>>>> else >>>>>> CUSTOM_MAKE_DIR_REL=NO_SUCH_PATH >>>>>> endif >>>>>> CUSTOM_MAKE_DIR=$(BUILDDIR)/$(CUSTOM_MAKE_DIR_REL) >>>>>> >>>>>> and then >>>>>> >>>>>> -include $(CUSTOM_MAKE_DIR)/<path-to-file.gmk> >>>>>> >>>>>> wherever we needed it (again initially only the two locations I >>>>>> mentioned). >>>>>> >>>>>> I'd like to hear people's opinions on this. >>>>>> >>>>>> Thanks, >>>>>> David >>>>> >>>
