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
