On 23/11/14 23:15, Magnus Ihse Bursie wrote:
On 2014-11-20 22:39, Chris Hegarty wrote:This is a review request for the changes for JEP 220: Modular Run-Time Images [1]. There are a number of individuals responsible for these changes. Some, possibly not all, are explicitly listed in the 'To' section of this mail, and they will help address any comments arising from this review request. The new run-time image structure is defined in JEP 220 [1], and can be seen as a result of building the staging forest [2], or in the early access builds [3]. Webrevs: http://cr.openjdk.java.net/~chegar/8061971/00/I have thoroughly reviewed all the build changes, and they look good to me, with the following remarks.
Thanks for taking the time to review the build changes Magnus.
It would have been nice to use the new "wrapper" based approach for all module-phase specific details -- this is done now for all phases except gensrc and rmic. But this is not necessary for the patch to work, and can be addressed later on. We also need to update configure to accept a module-based JDK as a boot JDK. In the patch, this is done sort-of -- we can run a bootcycle build with the new module-based JDK, but we will not accept it as an initial boot JDK. But this too can be fixed later on. And of course there is always some syntax issues. :-) In langtools/make/gensrc/Gensrc-jdk.*.gmk, there are several calls of the pattern: $(eval $(call SetupVersionProperties,JAVAP_VERSION, ...)) $(eval $(call SetupCompileProperties,COMPILE_PROPERTIES, ...)) These should have a space following the comma to adhere to the recommended style. But once again, I'm okay with fixing this in a follow up.
It is up to you. If some of these changes are benign, then maybe you could work with Erik to get them into jigsaw/m2, otherwise we can fix them in jdk9/dev.
Thanks again, -Chris.
