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 <[email protected]> > 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 <[email protected]> > > * 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 [email protected] +1-801-349-2682 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
