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.
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.)
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).
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. :-)
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.
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.
/Magnus
