On 2015-01-19 14:33, Erik Joelsson wrote:
Hello,
Please review this enhancement to the incremental build correctness. I
have implemented a way to track make dependencies to make variable
values, so that relevant changes to makefiles or configure will
properly trigger rebuilds of affected targets. See bug description for
more details.
Note that I've included new tests for these makefile features in the
file test/make/TestMakeBase.gmk. These can be run with the target
"test-make".
The macro MakeDir has been changed so that it no longer needs to be
called with $(eval ).
I have chosen to define the new macros with = assignment instead of
using "define", because they are meant to be called without $(eval ),
which means they can only be one logical line. Since "define" is used
to define multi line variables, it makes no sense to use it for these
macros.
Bug: https://bugs.openjdk.java.net/browse/JDK-8069261
Webrev: http://cr.openjdk.java.net/~erikj/8069261/webrev.01/
Erik,
Thanks for addressing this!
The patch looks overall good. Lots of karma for using the test framework! :)
Some minor comments.
The name standard of the new vardeps files seems a bit vague. :) I
suggest the following:
* In JavaCompilation:
$1_VARDEPS_FILE := $$(call DependOnVariable, $1_VARDEPS, $$(dir
$$($1_JAR))_the.$$($1_JARNAME)_vardeps)
Use .vardeps instead
* In MakeBase:
DependOnVariableFileName = \
$(strip $(if $(strip $2), $2, \
$(MAKESUPPORT_OUTPUTDIR)/vardeps/$(DependOnVariableDirName)/$(strip
$1).value))
Use .vardeps instead
* Also, in Tools.gmk the additions:
CFLAGS_windows := -nologo, \
seems to have crept in by mistake.
* In NativeCompilation, the line:
$$($1_RES): $$($1_VERSIONINFO_RESOURCE) $$($1_RES_VARDEPS_FILE)
is mis-indented
* Perhaps you can add some comment clarifying that $1_COMPILE_VARDEPS
contains exactly the variables that add_native_source depend on,
and that $1_COMPILE_VARDEPS_FILE is used by add_native_source?
Otherwise, it looks all very good!
/Magnus
