Hello,
Thanks for the comments. Here is a new version:
http://cr.openjdk.java.net/~erikj/8069261/webrev.02/
In addition to fixing the concerns below, I fixed the following:
* Added test-make to the all target so that it gets tested in JPRT. It
takes close to no time to run.
* Added 1 second sleeps in the tests when running on macosx, since the
filesystem on mac only has 1 second resolution. (WOW!)
* Changed WriteFile to use printf instead of echo since echo on Solaris
interprets things like '\t', which messes up comparisons.
* Reworked how DependOnVariable is used in JavaCompilation.gmk. Instead
of just blindly adding all input variables, I moved the definitions
closer to the recipes, like in NativeCompilation.gmk. I think this is
generally better, but the triggering reason was that too much unneeded
stuff got into the vardep variable value and the command line got too
long on Windows.
* For SetupArchive, I discovered that for vardeps changes, we have to
force a full recreation of the jar to be sure those changes are really
picked up. I also discovered that we currently didn't rebuild a jar if
the manifest template file was changed. (I'm happy I had test-make tests
for SetupArchive when fixing this.)
/Erik
On 2015-01-21 15:42, Magnus Ihse Bursie wrote:
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
