RFR [9] Modular Source Code

Magnus Ihse Bursie magnus.ihse.bursie at oracle.com
Wed Aug 20 10:14:53 UTC 2014

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!
           $$(foreach i,$$($1_ALL_COPIES),$$(eval $$(call 
       # Now we can depend on $$($1_ALL_COPY_TARGETS) to copy all files!

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

In make/common/support:
   The ListPathsSafely support files have changed, replacing the first 
three strings with:

   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 

In make/Main.gmk:
   I'm not entirely happy with the way the following dependencies are 
     # 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.


More information about the build-dev mailing list