patch 1/4 [was: [PATCH] take3: Add an implementation of gnulib's canonicalize_file_name for mingw.]

[Eric Blake]

On 02/04/2011 10:56 AM, Jan Nieuwenhuizen wrote:
> Hi,
> 
> See attached, please apply.

Can you use 'git send-email' or the like to thread your patches into
multiple mails?  There are enough unrelated issues that I'm finding it
hard replying to them all in one email.

>>From 98e49aefe607b2df4aa23401f219507b515a93f6 Mon Sep 17 00:00:00 2001
> From: Jan Nieuwenhuizen <address@hidden>
> Date: Fri, 4 Feb 2011 18:25:17 +0100
> Subject: [PATCH 1/4] canonicalize-lgpl: Add support for running without 
> Cygwin, off by default.

The commit message is inaccurate.  I can run test-canonicalize-lgpl on
Linux without Cygwin.  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 (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.
 But if it is, then I'd rather see it done globally to ALL gnulib tests
in one shot, and automating it at ./configure-time would be a bonus.

I would have written the commit summary like this:

tests: allow a fallback for missing rm (such as del on win32)

> 
> 2011-02-01  Jan Nieuwenhuizen  <address@hidden>
> 
>     * 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.

> +++ b/tests/test-canonicalize-lgpl.c
> @@ -37,6 +37,11 @@ SIGNATURE_CHECK (canonicalize_file_name, char *, (const 
> char *));
>  
>  #define BASE "t-can-lgpl.tmp"
>  
> +#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, and
ought to be hoisted into tests/macros.h to be easily shared among all
tests.  And to be namespace-safe, you probably ought to name it GL_RM_RF.

> @@ -56,7 +61,7 @@ main (void)
>       any leftovers from a previous partial run.  */
>    {
>      int fd;
> -    ignore_value (system ("rm -rf " BASE " ise"));
> +    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).

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

-- 
Eric Blake   address@hidden    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature