automake-patches
[Top][All Lists]
Advanced

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

Re: pr-msvc-support merge


From: Ralf Wildenhues
Subject: Re: pr-msvc-support merge
Date: Wed, 16 Jun 2010 20:57:56 +0200
User-agent: Mutt/1.5.20 (2010-04-22)

Hello Peter,

let's cut the libtool list from replies, ok?

* Peter Rosin wrote on Wed, Jun 16, 2010 at 02:30:47PM CEST:
> Den 2010-06-14 22:40 skrev Ralf Wildenhues:
> >* Peter Rosin wrote on Mon, Jun 14, 2010 at 09:35:45AM CEST:

> 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.

You have several ways to make your life easier: first, read
tests/README.  Use 'make -jN' for parallel execution.  All of Automake's
tests should complete without user interaction; if they don't, then we
will introduce check-{non,}interactive just like we did with Libtool.
You can use recheck to only check failed tests, and use TESTS=... for
subsetting as indicated before.

> >Some nits and questions:

> +path_conv=
> +
> +# func_path_conf build_path

Another remark: beside the typo in the function description s/conf/conv/
can we s/path/file/g globally on your patch?  The GNU Coding Standards
want us to speak of paths only for things like $PATH that consists of
potentially multiple 'file names'.  Thanks.

> >>+      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).

We rely on 'echo' not interpreting incoming backslashes here, right?
Is this something we need to be cautious over, or can 'cmd' only ever
find echo programs that work this way?  (IIRC all MinGW shells were
fixed to this end, but the echo called here is a stand-alone program
right?)

The backslash before $ should not be necessary.  The shell does not
expand a $ before a /, and the backslash makes me think you're trying
to sed-match a literal dollar.

> 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.

Yes, eval sounds very wrong, and more dangerous, too.

This explanation of yours lends itself nicely to a testsuite addition
that exercises the 'compile' script (no need to go through Autoconf or
Automake indirections), as in
  cp $testsrcdir/../lib/compile .
  ./compile ... "weird\path\that\has\backslash-t.c" ...
  ...

> >>+   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).

OK.

I have a more general question though: for all of 'cmd //c', cygpath,
and wineconv, are their conversions idempotent?  I.e., when I insert a
converted path, do they produce the same path again (a testsuite
addition could try to verify this).  This might be necessary because we
have other pending patches for libtool that convert names to host format
there already, and those should not be broken.

> >>+   -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.

Yep.

> 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).

Yep.

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

Yep, thanks.

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

This is good too, but using $nl is typically more instructive for the
untrained reader, and embedded newlines may be useful elsewhere, too.

BTW, the need to initialize IFS is because some shells might have it
unset by default on script startup; that is harmless in itself, but

  save_IFS=$IFS  # save_IFS is now an empty, but not unset variable
  ...
  IFS=$save_IFS  # empty now, field splitting is effectively disabled!

> --- a/NEWS
> +++ b/NEWS
> @@ -4,6 +4,9 @@ New in 1.11a:
>  
>    - automake now generates silenced rules for texinfo outputs.
>  
> +  - The `compile' script now converts some options for MSVC to make the
> +    user experience better.

Let's make that 'for a better user experience'.  'make' is such a bad
verb (sorry for the pun).

> +func_cl_wrapper ()
> +{
> +  # Assume a capable shell
> +  linker_opts=
> +  for arg
> +  do
> +    if test -n "$eat"; then
> +      eat=
> +    else
> +      case $1 in
> +     -o)
> +       # configure might choose to run compile as `compile cc -o foo foo.c'.
> +       eat=1
> +       case $2 in
> +         *.o | *.[oO][bB][jJ])

What about *.[oO]?  Is that something we need to take into account?

> +           func_path_conv "$2"
> +           set x "$@" -Fo"$path"
> +           shift

Thanks,
Ralf



reply via email to

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