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
