RFR: JDK-8069261: Create make dependencies on make variable values
Magnus Ihse Bursie
magnus.ihse.bursie at oracle.com
Fri Jan 23 12:34:57 UTC 2015
On 2015-01-23 12:17, Erik Joelsson wrote:
> Thanks for the comments. Here is a new version:
> 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.
While I do like to see the make tests being run more frequently, I'm not
sure I like the idea of adding them to the "all" target. I think
building and testing should be kept as separate as possible, even if it
matters testing the build system.
If your goal is to get it run in JPRT, why not add it to the JPRT
bundles target instead?
> * 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.)
Seems like it could be a good thing to add a test that changes to the
manifest file trigger a build? :-) (The current test seems to bundle
manifest updating with other changes which could by themselves trigger
> 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
>> Use .vardeps instead
>> * In MakeBase:
>> DependOnVariableFileName = \
>> $(strip $(if $(strip $2), $2, \
>> 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!
More information about the build-dev