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

[Erik Joelsson]

Actual link: http://cr.openjdk.java.net/~erikj/8189094/webrev.02/


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.
>> * 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 
>> INTERIM_LANGTOOLS_ARGS used?
> 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.
>>
>> * 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
>
> /Erik