The only issue I see is that it's using cygpath -m -a   and not   cygpath -s -m 
-a
which I think means that the path could have spaces in it.

Otherwise, looks fine.

-kto

On Feb 9, 2012, at 1:33 AM, Erik Joelsson 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.
> 
> Haven't changed anything regarding the nmake files.
> 
> /Erik
> 
> 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

Reply via email to