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

John Coomes John.Coomes at oracle.com
Thu Feb 9 22:43:03 UTC 2012

Erik Joelsson (erik.joelsson at oracle.com) wrote:

New webrev: http://cr.openjdk.java.net/~erikj/7141244/webrev.02/ <http://cr.openjdk.java.net/%7Eerikj/7141244/webrev.02/> 177 lines changed: 89 ins; 29 del; 59 mod; 3970 unchg

Changes since last time: * Moved the , to after $(SPEC) * Changed comment in gcc/sparcWorks.make according to suggestion from Fredrik.

Looks good. One minor request: in linux/makefiles/gcc.make, you moved the setting of STRIP under the SPEC conditional. Might as well fold it into the CROSS_COMPILE_ARCH conditional that's already there.

-John

On 2012-02-09 10:09, Erik Joelsson wrote: > > On 2012-02-09 03:51, David Holmes wrote: >> 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. >> > I guess it's an unconscious habit from java where you rather do > "".equals(something) to avoid NPE. I will switch it around. And the > assumption is correct. We used -include at first, but I figured that > we wanted to know if the include failed at least on the root level > Makefile. >> 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? >> > Grouping them together would certainly look nicer, but MT isn't set > for every possible compiler version. Not sure if that matters since we > don't support older versions anyway, right? > > As for testing for SPEC, this is nmake and the SPEC file is only > gnumake compatible. CXX, MT, RC and LD are sent in to nmake on the > command line from gnumake. They are then generated into local.make > which is in turn included by sub invocations of nmake. Sending in SPEC > as well seemed redundant to me, but we could send it in as a flag > signaling that configure should be in control. Wouldn't look obviously > better to me though. I'm open for suggestions. > > /Erik

More information about the build-dev mailing list