RFR: JDK-8069261: Create make dependencies on make variable values

[Magnus Ihse Bursie]

On 2015-01-23 12:17, Erik Joelsson wrote:
> 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.
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.)

Great! :-)

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 
the update.)

/Magnus

>
> /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
>