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

David Holmes david.holmes at oracle.com
Thu Feb 9 02:51:18 UTC 2012

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:

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

More information about the build-dev mailing list