[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: pr-msvc-support merge

From: Peter Rosin
Subject: Re: pr-msvc-support merge
Date: Wed, 16 Jun 2010 14:30:47 +0200
User-agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20100317 Thunderbird/3.0.4

Hi Ralf!

Den 2010-06-14 22:40 skrev Ralf Wildenhues:
[ adding automake-patches; this is ]

* Peter Rosin wrote on Mon, Jun 14, 2010 at 09:35:45AM CEST:
Den 2010-06-12 10:05 skrev Ralf Wildenhues:
Well, I sort of figured that the 'compile' script could end up absorbing
quite a bit of the cccl functionality so to make it unneeded.  But hey,
let's be honest, somebody would have to do this work, because I don't
have the resources to do it.

Do you think something along these lines would be acceptable? It would
remove the need of some patches on the pr-msvc-support branch...

The patch looks pretty good to me already, but is lacking additions for
ChangeLog, NEWS, and maybe doc/ and tests/ too.


[snip useful advice]

Running the tests are still an outright pain though, but I will try it.
But it might take some time, I'm not used to running them and msys
is...cumbersome. Meanwhile, I have attached the current version of the
patch in case there are further things to fix.

Some nits and questions:

+      case $path_conv in
+       mingw)
+         path=`cmd //C echo "$path " | sed -e 's/"\(.*\) " *\$/\1/'`

I fail to understand what this sed script is for.  Help?

It was the easiest I could come up with after experimenting a lot. That
wasn't yesterday though, but IIRC if you want to convert paths with
spaces, you need to quote the $path for cmd, hence the quotes in the
echo "$path " construct. The space before the end quote will make the
argument always contain a space which forces MSYS to add quotes when the
path is fed to the Windows process (cmd in this case). The quotes are
added by MSYS after converting the path to windows form. Without that
space, the string is only quoted if it happens to contain a space, so
view it as a canonicalizer. The sed script is there to remove those
quotes (and the space before the end quote). Also, something seem to
mysteriously add a space at the end, so I'm removing that too while at
it, but only if it's really there (it felt like a bug that might be
fixed at some point). It might be possible to use eval to remove the
quotes, but since the path will typically contain backslashes I didn't
want to go there.

+       cygwin)
+         path=`cygpath -w "$path"`

IIUC cygpath is pretty much required to be present on Cygwin
installations, right?  Can it fail though?  Should $path retain its old
value if it does?  Don't we want -m rather than -w for forward slashes
(which IIUC even MSVC programs should support) to avoid quoting issues?
   path=`cygpath -m "$path" || echo "$path"`

The mingw case and the wine case have backlashes, so in that case those
too have to be turned into forward slashes. But since this is the end of
the line (isn't it?), I don't really see the need to use forward slashes.
The only consumer of those paths is cl.exe (and friends).

+       wine)
+         path=`winepath -w "$path"`

winepath OTOH may not be present, so this should definitely fall back to
the unconverted path I think.  And maybe the path_conv-setting code
check for presence of winepath.

I added the wine stuff more as an afterthought, but adding that fallback
is easy enough. Done.

+       -L*)
+         func_path_conv "${1#-L}"
+         export LINK="$LINK -LIBPATH:$path"

Is LINK a predefined variable?  Does it come from libtool?  Or from the
user or the system?

There are three ways to get options through to the linker, IIUC. By
1) using the LINK environment variable. The user might have put
   stuff in it, so therefore preserve that and add any further
   options at the end.
2) using "-link <opt> ..." on the command line, but that has the
   disadvantage that *all* options after -link are fed to the
   linker. Hmmm, but here we have control over that, so using
   that approach is feasible.
3) using a response file, then you can feed options to the
   compiler and the linker in whatever order you like, but with
   the disadvantage that you need to clean up the (temporary)
   response file (costs a fork).

My libtool patches used #1, that is the only relation to libtool.
This new version uses #2 instead. I think that might be clearer?

+         ;;
+       -Wl,*)
+         arg=${1#-Wl,}
+         save_ifs="$IFS"; IFS=','
+         for flag in $arg; do
+           IFS="$save_ifs"
+           export LINK="$LINK $flag"
+         done
+         IFS="$save_ifs"

For this, IFS needs to be initialized to default near the beginning of
the script (there is an embedded TAB in the last line):

IFS=" "" "$nl"

Wasn't aware of that, thanks! But you have five quotes, that's wrong
I suppose, so I removed the one before $nl.

Is the nl variable needed though? I thought this looked good:
IFS=' ''        ''

+    shift
+  done
+  exec "$@"

There needs to be an 'exit 1' here, in case exec fails.

Good call, and thanks for the review!


Attachment: cl-wrapper-2.patch
Description: Text document

reply via email to

[Prev in Thread] Current Thread [Next in Thread]