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

[Ralf Wildenhues]

* Charles Wilson wrote on Fri, Aug 27, 2010 at 08:49:24PM CEST:
> On 8/26/2010 4:18 PM, Ralf Wildenhues wrote:
> >> +  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.

OK.  So then let that variable be named lt_cv_to_host_file_cmd then
(the _name is not really needed to understand it IMHO, and it is long
enough).  And document that.  But still, AC_SUBST it from configure.ac
so it doesn't leak into user Makefile.in files (where there is no use
for it whatsoever).

The thing that bugs me about this is a user API that exposes an internal
detail: the name of a function, possibly subject to change for
performance or whatever reasons (see my suggestion from yesterday).
There should ideally be an indirection between public naming and
internal usage, but I can't come up with one ATM.

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

So let's do it ASAP, thanks.  While at it, why not rename
  func_A_to_B_convert to func_convert_A_to_B

which would look a lot nicer too?

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

Tea, rather, but no.  ;-)

> You're right, this all belongs in the docu -- and it was placed here to
> help during development.

And that's fine, really.  Just not in the final code.

>  Development is mostly done now.  I'll remove
> this bit, and save it off to the side until I write the docu.

Great.

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

Ah.  Thanks for clearing that up.  In this case I suggest renaming the
function func_convert_msys_to_w32  (because w32 is what everyone will
understand as the format that you are meaning), and the comment as
something like

 # Automatic conversion to w32 is not available inside the cwrapper.

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

Point well taken.

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

Ahh, ok to keep it if you like.


> >> +# 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?

OK.

> >> +  $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?)

No, it is because eval of a completely single-quoted string is a nop.

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

Sure.

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

OK then.  It probably makes sense trying to optimize here.


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

OK here then (with names adjusted).

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

Oh that's plenty fine.

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

Why should you need the halt?  If there are new pushs in the meantime,
you just merge origin/master into your master, and push.  That way, your
actually tested commit is present as commit in the upstream tree (for
later bisection), but nobody needs to wait.  Let's use the power of git
to advantage for all!

Thanks,
Ralf