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
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.
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.
/Magnus
/Erik
