take 4: Add an implementation of canonicalize_file_name for MinGW

[Jan Nieuwenhuizen]

Hi,

Here's version 4 of my MinGW canonicalize_file_name fix in three
separate patches.

> Can you use 'git send-email' or the like

Sure, using git send-email now.

> The commit message is inaccurate.
> I would have written the commit summary like this:
> tests: allow a fallback for missing rm (such as del on win32)

Ok, reworded.

> What you are really doing is adding support for running on a system
> that does not meet the GNU Coding Standards minimum setup of
> providing rm

Yes, that's right.  I think that the GNU Coding Standards minimum
setup requirements apply only to the build system, what do they have
to do with the host system?

> (as Bruno pointed out earlier, the easiest ways to test for mingw
> are msys or cygwin cross-compilation, but it is possible to have
> rm.exe even without those environments).  I'm still not convinced
> that adding hacks to work around a missing rm is worth doing.

I would be very happy if you would include this hack: we would enable
hackers to fix MinGW code along with adding and running the required
tests, without having to tell them to buy a Microsoft Windows licence.

> But if it is, then I'd rather see it done globally to ALL gnulib
> tests in one shot

Sure, applied to all tests.

> >     * tests/test-canonicalize-lgpl.c (main): Add support for running
> >     without Cygwin by using CPPFLAGS='-DRM_RF="del /r/q"'.  Off by
> >     default.
> 
> Not sure you need to mention Cygwin here; merely mentioning that you are
> adding a fallback method to work around missing rm is enough.

Removed this mentioning of Cygwin.

> > +#ifndef RM_RF
> > +/* To run this test without Cygwin, use CPPFLAGS='-DRM_RF="del /r/q"' */
> > +#define RM_RF "rm -rf"
> > +#endif
> 
> Again, it would be nicer if you could figure out how to automatically
> set this at configure time rather than requiring user intervention,

How could configure figure out if the host system that you are going
to run this on will have or lack rm?  I'm leaving this suggestion for
now, a configure option can probably be added later, and I'm the only
one who needs it right now; I hope that's okay.

> and ought to be hoisted into tests/macros.h to be easily shared
> among all tests.

Done.

> And to be namespace-safe, you probably ought to
> name it GL_RM_RF.

Done.

> > +    ignore_value (system (RM_RF " " BASE " ise"));
> 
> If macros.h provides a default RM_RF, then I'm okay with this part of
> the patch, but it ought to be done to ALL the tests that use this idiom
> (I count 32 files).

Yes, that makes perfect sense.  Done.

> (Perhaps an unintended side effect, but I can use CPPFLAGS=-DRM_RF='":"'
> as a way to bypass removal altogether - sometimes, having an easy kill
> switch to avoid deletion of temporary files can be a nice feature for
> debugging purposes).

I have added this handy remark to the comment that goes with the macro
definition.

> > Subject: [PATCH 2/4] canonicalize-lgpl: Add basic sanity checks for mingw 
> > demonstrating general breakage.
> 
> I'm not a fan of summary lines longer than 80 columns.  I would have done:
> 
> canonicalize-lgpl: add mingw tests

Ok, reworded.

> >       * tests/test-canonicalize-lgpl.c (main)[(_WIN32 || __WIN32__) && ! 
> > __CYGWIN__]:
> 
> In the code, you need the full condition; but in changelog entries, I'm
> fine with abbreviating this particular idiom:
> 
> * tests/test-canonicalize-lgpl.c (main) [WIN32]: Add ...

Okay.  Of course, we try to avoid the use `WIN' in GNU, so I have
spelled out `WINDOWS'.

> For git bisectability, I don't like to commit known broken tests without
> also committing their fixes;

Okay, I have lumped my three nicely separated-out patches: the test,
the fix, and the removal of the debug printing, back together again in
one big patch.

> > +    fprintf (stderr, "BASE-canon: %s\n", result);
> 
> Oops - you've left debug statements in.  A successful test should be silent.
> 
> > +    fprintf (stderr, "CWD: %s\n", cwd);
> > +    result = canonicalize_file_name (cwd);
> > +    fprintf (stderr, "CWD-canon: %s\n", result);
> 
> Two more debug statements that should be removed.

Oh, didn't my '[PATCH 4/4] canonicalize-lgpl: Remove debug printing.´
remove all debug printing again?  I really thought it did, sorry!

Having debug printing in the initial, failing test was on purpose: my
initial bug report with patch was dismissed, it was suggested that
MinGW ran fine, a failing test case was demanded to go along with any
change of the current --imho completely useless-- implementation, and
insight was asked into what exactly was going wrong.  I figured that
adding debug printing and removing it in the same series of patches,
would ease this insight and so greatly higher my chances of getting
this into gnulib before the release of Guile 2.0.

> > +    /* Check if BASE's canonical name is somewhat canonical.  */
> > +    ASSERT ((strchr (result, '/') == NULL)
> > +         != (strchr (result, '\\') == NULL));
> 
> A canonical name for win32 should use ONLY \, never /.  Insist on that,
> otherwise, you won't be able to compare two path names that differ only
> in their choice of (otherwise-synonymous) directory separators.

Sure, the most important thing here is that all files that go through
canonicalize_file_name are consistent in their choice of slash or
backslash.  That is what this test is asserting: that either slash or
backslash is used consistently in file names, never both.

I have changed the default implementation to use backslashes, as this
seems to be your preference.  This can be changed to the behaviour
that I need by setting GL_PREFER_SLASH_INTERNALLY at compile time.

In LilyPond we use slash throughout, Guile also does that and I think
that most GNU (library-) code does.  Either way, I don't see much
benefit in cluttering the code with DIRSEP or / or \ checks.

> > +    ASSERT (result != NULL);
> 
> Where are you freeing result?  It's wise to avoid memory leaks, even in
> simple tests.

Ah, I'm often rather sloppy in tests.  It is of course nice if tests
show how to use the code.  Fixed.

Next thing is to get gnulib's putenv implementation for MinGW looked
at aend fixed (is there a reason not to drop gnulib's putenv entirely,
that fixes it for me?) and Guile 2.0.1 will be ready for
MinGW/Windows!

Thanks for your detailed review,
Greetings,

Jan.


--
Jan Nieuwenhuizen <address@hidden> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar®  http://AvatarAcademy.nl