On 2018-12-14 14:22, Magnus Ihse Bursie wrote:
Hi Erik,
I missed your comments a bit down in the message. Replies inline.
On 2018-12-14 19:23, Erik Joelsson wrote:
In configure today, we generate the SYSROOT_CFLAGS but we do not use
them in the configure script. Instead we rely on INCLUDE/LIB being
set. This is of course not ideal, but having to set
WSLENV=INCLUDE:LIB makes it more obvious. It would be better to
append SYSROOT_CFLAGS to CFLAGS for Cygwin and msys as well, but that
change is not required for WSL to work.
I see. I agree that we should change this behavior for all windows
envs to use SYSROOT_CFLAGS. But for now, I accept the WSLENV solution
then.
Replacing the path works for Cygwin, but not for WSL. At least not
the way we generate the VS_PATH in this patch. The VS_PATH will not
inherit the PATH from the WSL environment as base, it will get the
Windows PATH that was set before WSL was launched. We could perhaps
improve the extraction logic to make this work better. See below.
* This is a question as much to Erik as to you: is it really correct
to *always* rewrite VS_PATH to unix style? (I'm talking about lines
470..486 in toolchain_windows.m4) I think not; this sounds like it
will break cygwin. I think we should to this either conditionally on
us running on WSL, or we should convert it to a VS_WSL_PATH instead.
Or maybe I'm just missing something, I'm starting getting a bit
confused about all these paths and conversions...
In configure today, we do rewrite it, but it happens implicitly in
the extraction script. The current version of the patch looks like
what I posted. I will try to explain. In configure today, we generate
extract-vs-env.bat, which end up containing lines like this (in cygwin):
C:/cygwin64/bin/bash.exe -c 'echo VS_PATH=\"$PATH\" > set-vs-env.sh
(note the unbalanced '. This is baffling me, but currently works in
Cygwin.)
! :-o
The bat file calls back to bash to evaluate the env variable. When I
tried to get this to work for WSL, I had trouble getting the quoting
to work. I had also forgotten about WSLENV so $PATH would not be
translated, it would hold the default bash path, and for the other
variables (INCLUDE and LIB) they simply did not get values in bash.
My solution was to ditch bash here and just generate lines like this
instead:
echo VS_PATH="%PATH%" > set-vs-env.sh
This outputs the pure windows versions of the variables. For Cygwin,
the old construct resulted in an implicitly converted PATH variable
(though still with spaces!), but LIB and INCLUDE still pure windows.
This is why we already have the conversion loops for those below.
With the new construct, all variables remain pure Windows, which is
arguably more consistent.
So to work around now having pure windows paths in VS_PATH, I added
another rewrite loop. This is a bit inefficient, but has the benefit
of generating space safe paths in VS_PATH, which is a must if we are
to prepend them to FIXPATH.
I don't think we need to worry too much about efficiency in this case.
We have a lot of inefficiencies in the path handling on Windows. The
important thing is to get good and sane paths out of configure, so we
can work with them easily in make.
I just tried the patch on Cygwin and it isn't working. The new PATH
variable we create this way does not contain /usr/bin, but instead
/cygdrive/c/cygwin64/usr/bin, which doesn't actually exist. Cygwin fakes
/usr/bin internally, but if it's expressed as a /cygdrive path this
faking fails. The consequence is that make fails to find "gawk" later.
This variable extraction will need an overhaul before we can take i this
patch.
/Erik
