Re: [PATCH] [cygwin|mingw]: Add cross-compile support to cwrapper (take 6)

[Charles Wilson]

On 8/26/2010 4:18 PM, Ralf Wildenhues wrote:
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -528,7 +528,8 @@ TESTS_ENVIRONMENT = MAKE="$(MAKE)" CC="$(CC)" 
>> CFLAGS="$(CFLAGS)" \
>>      CXX="$(CXX)" CXXFLAGS="$(CXXFLAGS)" CXXCPP="$(CXXCPP)" \
>>      F77="$(F77)" FFLAGS="$(FFLAGS)" \
>>      FC="$(FC)" FCFLAGS="$(FCFLAGS)" \
>> -    GCJ="$(GCJ)" GCJFLAGS="$(GCJFLAGS)"
>> +    GCJ="$(GCJ)" GCJFLAGS="$(GCJFLAGS)" \
>> +    lt_cv_to_host_path_cmd="$(to_host_path_cmd)"
> 
> I don't understand why this change should be necessary.  In your
> testing, you describe that most setups set a correct to_host_path_cmd
> themselves.  For the other(s), you already describe
>   export lt_cv_to_host_path_cmd=override
>   ./configure ...
>   make all check
> 
> That should be sufficient without the above patch, no?

Well, IF you *export* that value before running configure, AND run the
tests from the same shell in which that value is exported, then sure,
that is all you need. (I think. It's been over two years since I wrote
this patch...)

However, we need to ensure that the tests work even if you run them from
a different shell that the one you configured with.  I realize we don't
tend to do this for *all* possible cache values that the end-user might
override, but this one (and the one Peter added) are special, because
the documentation is going to talk about them explicitly.  We're
actually going to *recommend* that certain classes of users override
these particular cache values.  That makes them different from all the
other lt_cv_* variables someone might override.

It's best to avoid surprises, in that case.

Anyway, I reviewed all of the past discussion, and I found where this
was introduced: it was the "addendum" to version 3:
http://lists.gnu.org/archive/html/libtool-patches/2009-01/msg00238.html

Now, while I admitted uncertainty back then about the Makefile.am
change, I recorded no other notes about the testsuite.at changes or why
I thought they were necessary.  I do remember originally implementing it
without those two changes, and then "having" to add first the
Makefile.am change, and then the testsuite.at change, before I could get
things to "work".  But, sadly, I no longer remember exactly WHAT the
failure mode was (maybe it had to do with the tests that re-run other
tests, like max_cmd_length?)

I'll try it without both changes (but with lt_cv_* exported in the
current shell) and see what happens.

***QQQ***
(above discussion)

>> --- a/libltdl/config/ltmain.m4sh
>> +++ b/libltdl/config/ltmain.m4sh
>> @@ -2775,166 +2775,595 @@ fi\
>>  "
>>  }
>>  
>> +####################################
>> +# PATH CONVERSION HELPER FUNCTIONS #
>> +####################################
> 
> Believe it or not, the GNU Coding Standards ask us to name things "file
> name" that which are files, and "path" those which are colon-separated
> (or $PATH_SEPARATOR-separated).  I'll close my eyes now on this issue,
> because I see hundreds of instances of it in this patch, so that'd be a
> TODO item.

It's not a difficult thing to do, and would be purely mechanical.  I can
put that on the queue for right after (or even before) the promised docu
patch.

It needs to happen before the release, because the docu will recommend
that certain users set lt_cv_to_host_path_cmd (ok,
lt_cv_to_host_file_name_cmd) to the name of one of these functions.  If
we're going to change the name of the functions, we better do it before
we document them.

>> +  lt_sed_naive_backslashify='s|\\\\*|\\|g;s|/|\\|g;s|\\|\\\\|g'
> 
> Might want to move this variable outside to just initialize it once?
> It's global anyway.  Maybe even general.m4sh.

It would make sense in general.m4sh.  OK.

>> +    func_wine_to_win32_path_tmp=`winepath -w "$1" 2>/dev/null`
>> +    if test "$?" -eq 0 && test -n "${func_wine_to_win32_path_tmp}"; then
> 
> I'll just note that some shells ((d?)ash 0.2) fail to propagate the exit
> status of an assignment.  No need to change the code, but users should
> have a decent shell for this.

OK.  I remember one of the earlier iterations actually invoked the
winepath command twice...I don't remember why, but maybe it was related.
Most shells -- especially on x86 linux where wine would actually be
available -- are decent, tho.

>> +      func_wine_to_win32_path_result=`$ECHO "$func_wine_to_win32_path_tmp" |
>> +        $SED -e "$lt_sed_naive_backslashify"`
>> +    else
>> +      func_wine_to_win32_path_result=
> 
> The way this is coded, correctness relies on the fact that all code
> paths that invoke this function do eventually check for non-emptiness
> of the result.

Yes. That is documented:
# ARG is the $build path to be converted to win32 format.
# result is available in $func_wine_to_win32_path_result
# result is empty on error (or when arg is empty)

>> +    func_wine_to_win32_pathlist_oldIFS=$IFS
> 
> IFS is saved and restored always to the same value in libtool, AFAIK,
> so a short variable name should suffice completely.

Ack.

>> +    IFS=:
>> +    for func_wine_to_win32_pathlist_f in $1; do
>> +      IFS=$func_wine_to_win32_pathlist_oldIFS
>> +      if test -n "$func_wine_to_win32_pathlist_f" ; then
> 
> This if is unnecessary, the for list does not expand $1 to empty tokens.

OK.

>> +# func_cygpath ARGS...
> 
> Wow.  That belongs in the documenation, not the code.  The code should
> have a concise text, how about:
> 
>   # func_cygpath ARGS...
>   # Wrapper around calling the cygpath program via LT_CYGPATH, when $host
>   # is *nix and Cygwin is hosted via a wine environment or MSYS.

It's also used in cygwin->mingw environments.

>   # Returns the Cygwin file name or path in func_cygpath_result, or an
>   # empty string on error.

This conversion is actually bidirectional; depending on whether the args
are '-m' or '-w' (to win32), or '-u' (to cygwin).

>   # ARGS are passed to cygpath, with the last one being the file name
>   # or path to be converted.
>   #
>   # Specify the absolute *nix (or MSYS) name to cygpath in the LT_CYGPATH
>   # environment variable, do not put it in $PATH.

OK.

>> +    func_error "LT_CYGPATH is empty or specifies non-existant file: 
>> \`$LT_CYGPATH'"
> 
> existent

Ack.

>> +# func_msys_to_win32 ARG
>> +# Converts ARG from msys (unix-ish) format to
>> +# win32 format. Can accomodate both paths and pathlists.
>> +# Result is available in func_msys_to_win32_result.
> 
> How about
> # Convert file or path ARG from MSYS format to w32 format.  Return
> # result in func_msys_to_win32_result.
> ?

OK.

>> +func_msys_to_win32 ()
>> +{
>> +  $opt_debug
>> +  lt_sed_naive_backslashify='s|\\\\*|\\|g;s|/|\\|g;s|\\|\\\\|g'
> 
> See above.

Ack.

>> +# Verify that ARG1 (a path in $build format) was
>> +# converted to $host format in ARG2. Otherwise, emit
>> +# an error message, but continue (resetting
>> +# func_to_host_path_result to ARG1).
> 
> Please reflow to 80 columns (or 78 or 72, but not 53, please).

Ack throughout. This was all just manual typing, reworded a hundred
times, so...I got tired of manual reflowing every time.  Now that we are
(or will be) happy with the wording, no problem.

>> +  if test -z "$4" && test -n "$3"; then
>> +    func_error "Could not determine the host path(s) corresponding to"
>> +    func_error "  \`$3'"
>> +    func_error "Continuing, but uninstalled executables may not work."
>> +    # Fallback. If even this fallback fails, the fix is not to
>> +    # complicate the expression below, but for the user to provide,
>> +    # in that situation, whatever elements are missing from the
>> +    # environment so that the actual pathlist conversion functions
>> +    # work properly (for instance, a working wine installation
>> +    # with winepath so that path translation in the cross-to-mingw
>> +    # case works).
> 
> This belongs in the documentation, no?

Well, it's really more of a warning to libtool developers: Don't try to
"fix" this.  But yes, something similar DOES belong in the
documentation, so that end users know what to do when they get the
warning message.

I'll shorten this text, and as part of the later docu patch add a
pointer from here to there.

>> +    if test "x$1" != "x$2"; then
>> +      lt_replace_pathsep_chars="s|$1|$2|g"
>> +      func_to_host_pathlist_result=`echo "$3" |\
> 
> unneeded trailing backslash.

Ack.

>> +func_pathlist_front_back_pathsep ()
>> +{
>> +  $opt_debug
>> +  case "$4" in
> 
> unneeded quotes

Ack.

>> +# invoked via `eval $to_host_path_cmd ARG'
> 
> eval without at least double-quoting following variables is almost
> always wrong.

Right. Will fix both this comment and invocation (remove eval).

>> +# At present, the following path conversions are supported:
>>  #    $build          $host
>>  #    mingw (msys)    mingw  [e.g. native]
>>  #    cygwin          mingw
>>  #    *nix + wine     mingw
>> +#    mingw (msys)    cygwin [*] [**]
>> +#    *nix + wine     cygwin [**]
> 
> Again, all of this belongs in documentation.  But I don't want to get
> too picky now, if it helps you developing then ok.

When I saw your comment above, I wondered how badly you'd spew orange
juice on your keyboard when you got here.

You're right, this all belongs in the docu -- and it was placed here to
help during development.  Development is mostly done now.  I'll remove
this bit, and save it off to the side until I write the docu.

>> +# converts the path ARG from $build format to $host
>> +# format.
> 
> Please reflow.

Ack.

>> +# A no-op path conversion function for use when $build == $host.
> 
> s/==/=/  for shell syntax.

Ack.

> 
>> +# or when there is no required (or known) conversion function
>> +# between $build and $host.
> 
> Or even better:
>   # func_noop_path_convert ARG
>   # Copy ARG to func_to_host_path_result.

Ack.

>> +func_noop_path_convert ()
>> +{
>> +  $opt_debug
> 
> You really want to turn on debugging here?  ;-)

I was on a roll.  You're right, this is ridiculous.

>> +# func_msys_to_mingw_path_convert ARG
>> +# A path conversion function for use with "native" mingw
>> +# builds -- that is, when $host is *mingw*, and $build
>> +# is *mingw* (which is to say, msys).
> 
> Please, globally s/(which is to say, msys)//.  IMVHO this will just
> confuse any user reading it besides Peter and you.  ;-)

OK.

>>  In this case, the
>> +# msys shell automatically converts paths for any non-msys
>> +# applications it launches, but that facility isn't available
>> +# from inside the cwrapper.
> 
> No need to describe what this function does not (and need not) do.

You're misunderstanding what that particular sentence is (trying to)
say.  Long form:

Normally, the msys shell itself will automatically convert paths that
are arguments to commands, from "unixish" to win32ish, when the command
itself is a win32 executable.

However, "inside" the cwrapper, we have to do the work manually, because
we can't rely on that facility.  So, we need this
func_msys_to_mingw_convert function, to perform the conversion
ahead-of-time, so that we can hardcode the win32 result in the cwrapper
source code.

So, the sentence you're objecting to is the "why" of the function, not
the "what".

***QQQ***
Should it still be removed? Or reworded?

> One reason I'm harping so much on all the comments is that, if they were
> concise, one could just see the actual code within one screenful and
> *see* what is different and immediately be able to guess why.

NP. I have no problem tightening things up now.  OTOH, it's a darn good
thing I *did* put all of this gunk in there the first time around,
because otherwise even I would have forgotten half of this stuff over
the last two years.

>> -# func_to_host_pathlist arg
>> +
>> +# func_cygwin_to_mingw_path_convert ARG
>> +# A path conversion function for use when $host is *mingw*
>> +# but $build is *cygwin*.
> 
> This is enough.

Ack.

>> +# func_msys_to_cygwin_path_convert ARG
>> +# A path conversion function for use when $host is *cygwin*
>> +# but $build is *mingw* (that is, msys). This implies running
>> +# a cross build from msys to cygwin -- but msys has notorious
>> +# problems executing cygwin apps, because of conflicts between
>> +# cygwin1.dll and msys-1.0.dll. However, we'll try it. First,
>> +# convert from msys to win32, then use func_cygpath to convert
>> +# from win32 to cygwin. Requires LT_CYGPATH.
> 
> I suggest:
> 
>   # Convert from MSYS to Cygwin.  Requires LT_CYGPATH set.

Ack.

> But who needs or wants this?  You didn't mention it in your testing
> AFAICS, and I don't understand why you wouldn't just use Cygwin if you
> wanted to have it anyway.  I'd just drop it, together with the
> associated case below.

This was the only way *I* could test the LT_CYGPATH stuff originally *as
an integrated part of libtool* since I didn't have a unix->cygwin
toolchain.  (I tested the LT_CYGPATH support "offline" on real unix by
extracting script fragments from libtool, and running them with a wine
environment; I tested it "online" back then using msys as a standin.)
But (a) that was all back in the cygwin-1.5.x days (when cygwin-1.5+wine
worked), and (b) now I actually have a working unix->cygwin compiler.
So, it was more of a testing and debugging tool back then.

The only qualm I have about dropping it NOW, is that with the new
problems with wine+cygwin-1.7, we AGAIN can no longer actually test the
LT_CYGPATH stuff on linux, even though I now have a unix->cygwin
toolchain: because cygwin-1.7+wine has...issues.  So, if we *really*
want to test LT_CYGPATH, we probably need the msys->cygwin environment
to do it.  But...that "testing mode" is the ONLY thing it would be good
for; nobody REALLY would ever want to build stuff that way, since cygwin
itself is a much better environment for building cygwin tools than an
old, out-of-date, slightly weird cygwin fork like MSYS.

***QQQ***
It's up to you, Ralf: what do you think? Drop it, or not?

>> +# func_nix_to_cygwin_path_convert ARG
>> +# A path conversion function for use when $host is *cygwin*
>> +# but $build is some *nix variant. In this case, we assume
>> +# that a wine environment with a working winepath executable
>> +# is available in $build's $PATH, and that cygwin is installed
>> +# within that wine environment. Requires LT_CYGPATH (see
>> +# func_cygpath).
> 
>   # Convert file name ARG from *nix to Cygwin.  Requires Cygwin
>   # installed in a wine environment, working winepath, LT_CYGPATH.

Ack.

>> +#################################################
>> +# $build to $host PATHLIST CONVERSION FUNCTIONS #
>> +#################################################
>> +# invoked via `eval $to_host_pathlist_cmd ARG'
> 
> All comments above apply to this section as well.  Below I'm only
> annotating diffs.

Ack.

>> +# ASSUMPTIONS: all such conversion functions are
>> +# named using the following convention:
>> +#   path conversion function    : xxxxxx_path_convert ()
>> +#   pathlist conversion function: xxxxxx_pathlist_convert ()
>> +# where, for any given $build/$host combination the 'xxxxxx'
>> +# value is the same.
> 
> What do you mean with ASSUMPTIONS?  Either the code does that, or it
> doesn't.  The comment is either tautological with the code (and thus can
> be removed), or it is wrong, in which case the code should be fixed.
> Thanks.

The code is structured the way the comment describes.  The idea was that
if someone were to add a new xxxxxx_path_convert for some new build/host
combination (which in the future will be globally renamed to
xxxxxx_file_name_convert) function, then they need to ALSO add an very
similarly named xxxxxxx_pathlist_convert (aka to be globally renamed
xxxxxx_path_convert).

If they don't follow these rules, then stuff will break.  This was so
that I could get away with using only a single lt_cv_ variable, instead
of two.

***QQQ***
How about I reword the comment to make it clear that (a) it is
describing the pairing between members of two families of functions, and
(b) newly added members of those families must follow the same naming
convention?

>> +  $opt_debug
>> +  func_init_to_host_pathlist_cmd
>> +  eval '$to_host_pathlist_cmd "$1"'
> 
> This should work:
>   $to_host_pathlist_cmd "$1"

Ack.

(To make sure I understand, this change is ok because the variable
to_host_pathlist_cmd will never itself contain other, unexpanded
variables, right?)

>> +}
>> +# end func_to_host_pathlist
> 
>> +# func_noop_pathlist_convert ARG
>> +# A no-op pathlist conversion function for use when $build == $host,
>> +# or when there is no required (or known) conversion function
>> +# between $build and $host.
> 
> See above (other noop function).

Ack.

>> +# func_cygwin_to_mingw_pathlist_convert ARG
>> +# A pathlist conversion function for use when $host is *mingw*
>> +# but $build is *cygwin*. In this case, the cygpath program
>> +# provided by the $build environment is sufficient for all
>> +# conversions.
> 
> The second sentence is not needed.

Ack.

>> +# ARG is the pathlist to be converted; the result is available
>> +# in func_to_host_pathlist_result.
>> +func_cygwin_to_mingw_pathlist_convert ()
>> +{
>> +  $opt_debug
>> +  func_to_host_pathlist_result="$1"
>> +  if test -n "$1"; then
>> +    # Remove leading and trailing path separator characters from
>> +    # ARG. msys behavior is inconsistent here, cygpath turns them
>> +    # into '.;' and ';.', and winepath ignores them completely.
> 
> This comment can just be
>        # See func_msys_to_mingw_pathlist_convert.
> 
> Repetition always leads to maintenance problems.

OK.

>> +# func_nix_to_mingw_pathlist_convert ARG
>> +# A pathlist conversion function for use when $host is *mingw*
>> +# but $build is some *nix variant. In this case, we assume
>> +# that a wine environment with a working winepath executable
>> +# is available in $build's $PATH.
> 
> see above.

Ack.

>> +# ARG is the pathlist to be converted; the result is available
>> +# in func_to_host_pathlist_result.
>> +func_nix_to_mingw_pathlist_convert ()
...
>> +    # Remove leading and trailing path separator characters from
>> +    # ARG. msys behavior is inconsistent here, cygpath turns them
>> +    # into '.;' and ';.', and winepath ignores them completely.
> 
> see above.

Ack.

>> +# func_msys_to_cygwin_pathlist_convert ARG
>> +# A pathlist conversion function for use when $host is *cygwin*
>> +# but $build is *mingw* (that is, msys). This implies running
>> +# a cross build from msys to cygwin -- but msys has notorious
>> +# problems executing cygwin apps, because of conflicts between
>> +# cygwin1.dll and msys-1.0.dll. However, we'll try it. First,
>> +# convert from msys to win32, then use func_cygpath to convert
>> +# from win32 to cygwin. Requires LT_CYGPATH.
> 
> See above and elsewhere above.

Ack.

>> +func_msys_to_cygwin_pathlist_convert ()
...
>> +    # Remove leading and trailing path separator characters from
>> +    # ARG. msys behavior is inconsistent here, cygpath turns them
>> +    # into '.;' and ';.', and winepath ignores them completely.
> 
> See above.

Ack.

>> +    func_pathlist_convert_check ":" ":" \
> 
> No need to quote :

OK.

>> +# func_nix_to_cygwin_pathlist_convert ARG
>> +# A pathlist conversion function for use when $host is *cygwin*
>> +# but $build is some *nix variant. In this case, we assume
>> +# that a wine environment with a working winepath executable
>> +# is available in $build's $PATH, and that cygwin is installed
>> +# within that wine environment. Requires LT_CYGPATH (see
>> +# func_cygpath).

similar changes here, I presume.

>> +func_nix_to_cygwin_pathlist_convert ()
...
>> +    # Remove leading and trailing path separator characters from
>> +    # ARG. msys behavior is inconsistent here, cygpath turns them
>> +    # into '.;' and ';.', and winepath ignores them completely.
> 
> See above

Ack.

>> +    func_pathlist_convert_check ":" ":" \
> 
> No need to quote :

OK.

> I still fail to understand why the file name and path variants of the
> functions cannot be collapsed pairwise.  Oh well.

Think of it as refactoring.  If there were just a single function that
accepted both file names and paths, it would look like this (with the
exception of cygwin->mingw, since cygpath can be used for both inputs;
you just have to add '-p' if passing a path).

if path
  strip leading/trailing seps
  play games with IFS
  loop over file names
     <lots of code to handle converting the file name>
  endloop
  fix IFS
  add leading/trailing seps
else (file name)
  <lots of code to handle converting the file name>
fi

I'm leaving out the error checking and handling, but basically you'd
duplicate all the core conversion code.  So, I pulled it out into
separate functions.

It would be nice if all of the "_pathlist" functions could be
consolidated into a *single* wrapper around whatever
$func_to_host_path_cmd specifies, but...I couldn't figure out a good way
to do that; there are just enough differences even in how the
pseudo-code in the "path" section above has to be implemented on the
various platforms, that it is difficult.  I did try to abstract a lot of
the code into common helper functions, so at least most of the _pathlist
functions are relatively straightforward.

See also here:
http://lists.gnu.org/archive/html/libtool-patches/2009-01/msg00237.html
which is part of a discussion of version 3 of this patch (search for "In
general, converting pathlists is a strikingly" to get to the relevant
section; later revisions -- including this one -- did implement the
simplificatiosn described near "some consolidation possible").

>> --- a/libltdl/m4/libtool.m4
>> +++ b/libltdl/m4/libtool.m4
> 
>> +# _LT_PATH_CONVERSION_FUNCTIONS
>> +# -----------------------------
>> +# Determine which path conversion functions should be
>> +# used by func_to_host_path (and, implicitly, by
>> +# func_to_host_pathlist).  These are needed for certain
>> +# cross-compile configurations and "native" mingw (which
>> +# is actually an msys->mingw cross).
> 
> Please reflow.  Please don't explain *here* what "native" MinGW is,
> define it once in the manual instead.

OK.

>> +m4_defun([_LT_PATH_CONVERSION_FUNCTIONS],
>> +[AC_REQUIRE([AC_CANONICAL_HOST])dnl
>> +AC_REQUIRE([AC_CANONICAL_BUILD])dnl
>> +AC_MSG_CHECKING([how to convert $build paths to $host format])
>> +AC_CACHE_VAL(lt_cv_to_host_path_cmd,
>> +[case $host in
>> +  *mingw* )
> 
> The usual matching is either
>   case $host_os in
>     mingw*) ... ;;
>     cygwin*) ... ;;
> 
> or
>   case $host in
>     *-*-mingw*) ... ;;
>     *-*-cygwin*) ... ;;
> 
> so that you actually match the OS part of the triple.  Several
> instances.

Ack.

>> +_LT_DECL([to_host_path_cmd], [lt_cv_to_host_path_cmd],
>> +         [0], [convert $build paths to $host format])dnl
>> +AC_SUBST([to_host_path_cmd])dnl
> 
> This AC_SUBST seems wrong.  We don't want
>   to_host_path_cmd = func_nix_to_cygwin_path_convert
> 
> to appear in user packages' Makefile files.  You want the AC_SUBST
> in Libtool toplevel's configure.ac instead, so that it appears in
> Libtool's toplevel Makefile.

I see.

>> --- a/tests/testsuite.at
>> +++ b/tests/testsuite.at
...
>> @@ -45,6 +45,9 @@ fi
>>  if test -n "$build_alias"; then
>>    configure_options="$configure_options --build $build_alias"
>>  fi
>> +if test -n "$to_host_path_cmd"; then
>> +  configure_options="$configure_options 
>> lt_cv_to_host_path_cmd=$to_host_path_cmd"
>> +fi
>>  if (FOO=bar; unset FOO) >/dev/null 2>&1; then
>>    unset=unset
>>  else
> 
> I don't see why this change should be necessary (see my other comment
> regarding 'export lt_cv_to_host_path_cmd' above).

For the same reasons as I explained above. (still an open question).

> I haven't tested the patch.  I suppose it's more productive to test the
> end result.

Well, sure.

> To let you not feel that I am making this another neverending story:
> please, after revising, if you don't have open questions, 

There are a few open questions above; pls advise. (each is marked by
***QQQ***).

> do commit the
> patch, judging yourself whether it is suitable for master yet or, if you
> deem it not, just push it to some new branch, let's call it
> file-conversion for lack of a better name.

As I said, I don't mind rebasing.  I'll probably only test the result on
cygwin-native, mingw-native, and linux->mingw, and then push to master.
 However, once I have finished the requested changes above and the
rebasing (plus whatever comes of the four open ***QQQ***uestions), I
might ask for a 12 hour halt on updates to master so I can run those
tests, if that's ok?

--
Chuck