Review for 7141244: build-infra merge: Include $(SPEC) in makefiles and make variables overridable

[David Holmes]

On 9/02/2012 1:36 AM, Erik Joelsson wrote:
> New webrev up with the changes I detailed below:
>
> http://cr.openjdk.java.net/~erikj/7141244/webrev.01/
> 177 lines changed: 89 ins; 29 del; 59 mod; 3970 unchg

make/defs.make:

+ ifneq (,$(SPEC))
+   include $(SPEC)
+ endif

Having the blank first looks odd. I assume you aren't using -inlcude 
because you want to see errors if SPEC is set but not found.

make/windows/makefiles/compile.make:

The definitions of MT=mt.exe in each block for the different VS versions 
seems redundant. If we factor this out is there any reason not to group:

CXX=cl.exe
MT=mt.exe
RC=rc.exe
LD=link.exe

together and use the same "if (,$(SPEC))" approach?

David
-----

> /Erik
>
> On 2012-02-08 09:14, Erik Joelsson wrote:
>> Thanks for looking at this!
>>
>> On 2012-02-08 02:47, David Holmes wrote:
>>> It doesn't make sense to me to include SPEC in make/Makefile and
>>> make/Defs.make because Makefile includes Defs.make. You only need the
>>> -include in Defs.make (unless SPEC is going to define GAMMADIR or
>>> ALT_OUTPUTDIR - in which case include it in the Makefile not Defs.make)
>>>
>> I checked this again and you are right, we don't set anything that
>> warrants including SPEC in make/Makefile. I will move it to defs only.
>>> So this seems really ugly to me. If these were all set as Make
>>> variables on a top-level make invocation then you wouldn't need to do
>>> any of these tests. If the SPEC file is always going to set these
>>> variables then why not either include SPEC or else do these
>>> definitions eg:
>>>
>>> ifeq ($(SPEC),)
>>> CC = ...
>>> CXX = ..
>>> ...
>>> endif
>>> # else SPEC already defined these
>>>
>>> this might need some refactoring to group the necessary settings
>>> together.
>>>
>> This was how I initially did it, but I wasn't sure on the best
>> solution. I also forgot about command line overriding normal
>> assignments. With an explicit check for SPEC it's very obvious what we
>> are trying to achieve. I will look into this and try to group things
>> more neatly together for it. Hope to publish a new webrev later today.
>>
>> /Erik