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
