Request for Review: Add hotspot support for building with mingw/msys on build-infra (the new build)

[Kelly O'Hair]

On Oct 26, 2012, at 1:04 AM, Magnus Ihse Bursie wrote:

> On 2012-10-26 03:24, Kelly O'Hair wrote:
>> It has been my experience that using cygpath -w created problems with shell escape logic,
>> and that is why cygpath -m with a make level subst change is done, it was less problematic.
> It think it would work, but I understand why you are cautious. I can revert that part.
>> I don't understand why you would even want to change the cygwin logic at all, if it isn't broken why mess with it?
> It turned out that it was indeed broken, but I had forgot what I fixed. :-) The current code works if CC is defined like
> CC=/path/to/compiler
> but it will not work if CC is defined like
> CC=/path/to/compiler -extra -compiler -options
> That is what the $(firstword) stuff is about, just converting the compiler path.

But according to the GNU manual, http://www.gnu.org/software/make/manual/make.html#Implicit-Variables,
the CC name should be just a path to an executable, at least by convention, compiler flags belong in other variables.

> 
> This would be just a complaint in general that it broke expectations on how to setup CC, if not this turned out to be a problem for the latest changes in build-infra.
> 
> In older build-infra, we send in
> CC=/path/to/fixpath /path/to/compiler
> and by a lucky co-incidence, this will actually work, altough not as expected. (As far as I can understand, the hotspot makefile cygpath rewrite will change both paths.)

What "hotspot makefile cygpath rewrite" are we talking about here?

The CYGWIN cygpath utility, as far as I know, operates on one path or a PATH style path (path:path), not a
command line or series of blank separated paths.

> 
> However, when I added support for msys in build-infra, I changed the syntax for fixpath (formerly uncygdrive). It now requires an switch telling it to work in cygwin or msys mode, so we send in:
> CC=/path/to/fixpath -c /path/to/compiler
> which will make the hotspot rewrite code break. (My guess is that cygpath converts /path/to/fixpath but ignores everything from -c and forward).

If cygpath is given a command line, I think we may be in 'undefined' behavior, I'm not sure what it would do.

As I understand it, the uncygdrive (and fixpath) tool is intended to process a full command line, right?
But CYGWIN's cygpath utility is just for paths.  Or am I missing something here?

> 
> Since fixpath actually isn't technically needed when building hotspot, we can work around this differently in Hotspot. Maybe that is the pragmatic way. But I still believe it's theoretically wrong with not handling a CC= which contains more than a path to an executable. :-)

I tend to disagree, having multiple words in CC is unconventional, and if we do that, we need to consider the
consequences of straying from the typical make convention.
But I'd rather not get into what is theoretically wrong or right, it just needs to work, so we do what we have to.

The fixpath trick concerns me on windows, all the extra exec's and all, but I've kept quiet about it because I'm
not that up to speed on the specific issues. I think we need to watch fixpath pretty closely, something tells me
we aren't done with this issue, just a gut feeling.

> 
> 
>> So my suggestion would be to add the msys logic, and leave the old cygwin logic alone.
>> 
>> I could have sworn that Tim Bell already did this change, and yes, if I look at:
>> 
>>     http://hg.openjdk.java.net/jdk8/build/hotspot/file/dccd40de8db1/make/windows/makefiles/defs.make
>> 
>> These changes are there already, or something equivalent.
>> 
>> Have you talked with Tim Bell about this change?
> 
> Not specifically about this change, but we communicated a lot while I was developing msys support for build-infra.
> 
> I'm not sure what you want to tell me by the link. There was a change (7181175: Enable builds on Windows with MinGW/MSYS),
> http://hg.openjdk.java.net/jdk8/build/hotspot/rev/0d8e265ba727
> which added the USING_MINGW variable that I need to check. However I can't see anything in this file that would fix the CXX etc variables for msys, the way my patch is doing it.
> 

You are correct that something needs to fix the multiple word issue of $(CC) for msys.

> But then again, we can probably do without this patch, if we make things differently in build-infra. We're already doing some weird special stuff for Hotspot builds that I guess adding another exception won't be that different. I'll look into it.

I appreciate that.

-kto

> 
> /Magnus