RFR: JDK-8189094: Change required boot jdk to JDK 9

Magnus Ihse Bursie magnus.ihse.bursie at oracle.com
Wed Oct 18 08:34:10 UTC 2017

On 2017-10-17 15:59, Erik Joelsson wrote:
> New webrev:
> On 2017-10-17 13:10, Magnus Ihse Bursie wrote:
>> This is a quite complex change. It's a bit unfortunate that we have 
>> to make these kinds of changes to use JDK 9 as a boot JDK. Anyway, 
>> that's the way it is.
>> A couple of remarks:
>> * In boot-jdk.m4, please update the comment as well:
>>     # When compiling code to be executed by the Boot JDK, force jdk8 compatibility.
>> - BOOT_JDK_SOURCETARGET="-source 8 -target 8"
>> + BOOT_JDK_SOURCETARGET="-source 9 -target 9"
> Fixed
>> * In JavaCompilation.gmk:
>> + # exist yet, is in it.
>> + $$(foreach d,$$($1_SRC), \
> Fixed
>> You can add a space after the comma with no ill effects.
>> - $$(eval $$(call FillCacheFind,$$($1_SRC)))
>> + $$(eval $$(call FillCacheFind, $$(wildcard $$($1_SRC))))
>> Should not the $(wildcard) filtering be done in the FillCacheFind 
>> function? It seems reasonable that it should not complain with an 
>> error if you give it a non-existing directory.
>> - $1_ALL_SRCS += $$(foreach s, $$($1_SRC), $$(call CacheFind, $$(s)))
>> + $1_ALL_SRCS += $$($1_EXTRA_FILES) $$(foreach s, $$($1_SRC), $$(if 
>> $$(wildcard $$s), \
>> + $$(call CacheFind, $$s)))
>> The same goes here. Any wildcard filtering should be done in 
>> CacheFind, I think.
> I looked at CacheFind and figured it didn't already do wildcard 
> consistently so opted to change the call site. In the new patch I 
> added wildcard checking in all instances of CacheFind instead. This 
> means we likely have places where we can remove redundant wildcard 
> calls in other places. Removing those in this change is starting to 
> feel like too much however. I can file a followup.
Please do. I made a quick check, and it looks like it's at least 10 
places we can simplify.
>> * In spec.gmk.in, the code for INTERIM_LANGTOOLS_MODULES_COMMA is 
>> basically repeating the pre-existing macro CommaList. However, that 
>> is only available in MakeBase.gmk. I'm not sure it's possible to use, 
>> but maybe. It looks like there's a lot of hard-coded stuff in 
>> spec.gmk.in, and maybe that is not the right place for it. Where are  
> The SPEC is always included first, so the CommaList macro is not 
> available. I agree that the spec is not necessarily the optimal place 
> to put static things like this, but we don't really have a Common or 
> Defs to put such things in. The args are used whenever we need to run 
> the interim langtools, including compiling java, running javadoc and 
> some gensrc/gendata. Some of the other variables are needed when 
> compiling the interim modules.
> In my defense I'm not changing the location of these settings in the 
> patch.
Fair enough. Maybe we should have a make/common/Constants.gmk or 
something like that which is included first in the spec?

>> * In jib-profiles.js:
>> You have no "var" declaration for the new boot_jdk_version etc. I'm 
>> not too well-versed in javascript and it is probably quite legal not 
>> to, but I think it's better style at least to keep it.
> Fixed
>> * Finally, $(BUILDTOOLS_OUTPUTDIR)/override_modules should be renamed 
>> "interim_modules".
> Fixed

Your fixed webrev looks good to me.


> /Erik

More information about the build-dev mailing list