RFR [9] Modular Source Code

[Magnus Ihse Bursie]

On 2014-08-15 10:52, Magnus Ihse Bursie wrote:
> These are indeed the single most significant changes to the build 
> system since the "new" build system was introduced.
>
> Here follows a partial review of the changes in the jdk repo. Even 
> though it's long, I'm not done. ;-)

While this has turned into a post-factum code review, I continue 
unabashed. :-)

In langtooks/make/GensrcLangtools.gmk:
   The fix from 8026773 was inadvertently reverted in the following line:
     $(ECHO) Compiling $(words $(PROPSOURCES) v1 v2 v3) properties into 
resource bundles
   This might implicate a merge error at some point, so maybe the file 
should be more thoroughly studied if there are more parts of the fix for 
8026773 that has been lost.

In make/common/JavaCompilation:
   The following stanza is incorrectly indented:
     ifneq (,$$($1_ALL_COPIES))
       # Yep, there are files to be copied!
       $1_ALL_COPY_TARGETS:=
           $$(foreach i,$$($1_ALL_COPIES),$$(eval $$(call 
add_file_to_copy,$1,$$i)))
       # Now we can depend on $$($1_ALL_COPY_TARGETS) to copy all files!
     endif

In make/CompileJavaModules:
   A typo:
     ## Service types are required in the classpath when compiing 
module-info

In make/common/support:
   The ListPathsSafely support files have changed, replacing the first 
three strings with:
     s|X01|share/classes|g
     s|X02|internal|g
     s|X03|com/sun/org|g

   While the old replacements did not do anything useful ("shortening" 
three-letter strings into another three-letter string), this change 
breaks the old pattern of having the strings sorted in length order. At 
first, I thought it was just a matter of style (I still think this is a 
matter of style, and that it should be fixed) but then I realized there 
also was a reason for this:

   The strings are matched from longest to shortest (given, of course, 
that the list is correctly ordered) and thus the string "com/sun" will 
match and be replaced by X07 before the new string "com/sun/org" will be 
given the chance.

In corba/make and langtools/make:
   For clarity and consistency, the file corba/make/CompileCorba.gmk 
should be renamed CompileInterimCorba.gmk and 
langtools/make/CompileInterim.gmk should be renamed 
CompileInterimLangtools.gmk.

In make/Main.gmk:
   I'm not entirely happy with the way the following dependencies are 
encoded:
     # Declare dependencies from all other <module>-lib to java.base-lib
     $(foreach t, $(filter-out java.base-libs, $(LIB_TARGETS)), \
         $(eval $t: java.base-libs))
     # Declare the special case dependency for jdk.deploy.osx where libosx
     # links against libosxapp.
     jdk.deploy.osx-libs: java.desktop-libs

     # This dependency needs to be explicitly declared. jdk.jdi-gensrc 
generates a
     # header file used by jdk.jdwp libs.
     jdk.jdwp-libs: jdk.jdi-gensrc

   I have discussed this off-line with Erik and concluded that this 
probably has to do for now, but I believe there should be a better way 
of describing this kinds of relationships.


Alright, now I'm done code reviewing! :-) Lots of kudos to Erik for 
managing to re-create the build system for the modular source code with 
such high quality.

/Magnus