Re: [PATCH] Add an implementation of gnulib's canonicalize_file_name for mingw.

[Eric Blake]

On 02/01/2011 06:59 AM, Jan Nieuwenhuizen wrote:
> It's good to see how thorough you are, even though I still think
> this is not much of a change as much as a first implementation.
> 
> See attached, greetings,

> Bruno Haible schreef op di 01-02-2011 om 10:46 [+0100]:
> 
> Hi Bruno,
> 
>> > In which respect did you find it useless for mingw?
> In that, when given an absolute file name, such as
> 
>    c:/program files/lilypond/usr/share/guile/2.0/ice-9/boot-9.scm
> 
> it returns NULL.

Fixing that would indeed be nice, which does mean that mingw needs a
separate implementation distinct from the Unix way of doing things.

>  It also returns NULL when given the CWD or a
> simple drive letter, such as C: or C:/.  When given a local file
> name "baz", it returns something like
> 
>     C:\foo\bar/baz
> 
> which is certainly not canonical.

But not incorrect, since windows treats / and \ synonymously.  But yes,
having a canonical form that always favors \ would be nice.

> +++ b/gnulib-tool
> @@ -5736,7 +5736,7 @@ s/\([.*$]\)/[\1]/g'
>      cd "$destdir"
>        mkdir build
>        cd build
> -        ../configure || func_exit 1
> +        ../configure "$CONFIGURE_ARGS" || func_exit 1

Why is this quoted?  It incorrectly passes the empty string as an
argument if CONFIGURE_ARGS is not provided in the environment.  I like
the idea of being able to provide overrides like this; but the current
practice is that you instead do:

./gnulib-tool --create-testdir --dir=xyz --with-tests module

at which point you then have a self-contained directory that can be
copied to any other machine (perhaps via NFS) and run ./configure
--args-of-your-choice in that directory.  That is, ./gnulib-tool --test
is shorthand for the default case that creates the directory and runs
configure with no arguments, but you can split the two steps when you
want to do more detailed testing.

> Test output without this fix:
> 
>     13:00:24 address@hidden:~/vc/gnulib/mingw/build
>     $ LANG= gltests/test-canonicalize-lgpl.exe
>     wine: cannot find L"C:\\windows\\system32\\rm.exe"

NACK.  We prefer to cater to mingw rather than wine (since wine often
has bugs that pure windows does not), and we require the bare minimum
set of basic utilities as required by the GNU Coding Standards be
present before running tests (that is, blindly assuming that rm is
installed is appropriate - you haven't properly set up mingw if it is
not available).

That, and if you can still manage to convince me that this is a good
change, then you need to make it to a lot more test files than just
test-canonicalize-lgpl.c.

> +++ b/tests/test-canonicalize-lgpl.c
> @@ -56,8 +56,13 @@ main (void)
>       any leftovers from a previous partial run.  */
>    {
>      int fd;
> +#ifndef __MINGW32__

Not to mention that this is not the proper way to test for a native
windows (or even wine) compilation environment.

> Test output without this fix:
> 
>     13:17:39 address@hidden:~/vc/gnulib/mingw/build
>     $ LANG= gltests/test-canonicalize-lgpl.exe
>     ise : File Not Found
>     mkdir:-1
>     fixme:msvcrt:MSVCRT__sopen : pmode 0x60fd88 ignored

Hmm, these three lines do not appear under a mingw run; which means this
might be a wine bug.

>     skipping test: symlinks not supported on this file system

But the test is successfully skipped, not failed, so I'm not sure about
this patch in isolation.

> +++ b/tests/test-canonicalize-lgpl.c
> @@ -103,6 +103,8 @@ main (void)
>      ASSERT (errno == ENOENT);
>    }
>  
> +#ifndef __MINGW32__
> +
>    /* From here on out, tests involve symlinks.  */
>    if (symlink (BASE "/ket", "ise") != 0)

Actually, the proper way is to test HAVE_SYMLINK, not ifndef
__MINGW32__; see how test-readlinkat.c skips symlink-related tests for
mingw.

> +     * tests/test-canonicalize-lgpl.c (main)[__MINGW32__]: Skip cleanup using
> +     remove.  Fixes aborting on cleanups.  We do an initial cleanup anyway.
> +
> +2011-02-01  Jan Nieuwenhuizen  <address@hidden>
> +
>       * tests/test-canonicalize-lgpl.c (main)[__MINGW32__]: Skip symlink
>       tests.  Fixes aborting on symlink tests.
>  
> diff --git a/tests/test-canonicalize-lgpl.c b/tests/test-canonicalize-lgpl.c
> index f80b55d..49f8643 100644
> --- a/tests/test-canonicalize-lgpl.c
> +++ b/tests/test-canonicalize-lgpl.c
> @@ -103,7 +103,9 @@ main (void)
>      ASSERT (errno == ENOENT);
>    }
>  
> -#ifndef __MINGW32__
> +#ifdef __MINGW32__
> +  return 0;
> +#endif /* __MINGW32__ */

This just undid the previous patch.  The real problem here is that the
cleanup is assuming that particular files exist, but they don't if
symlink support was missing; rather than an early exit here, the better
fix is to makes sure that symlink tests are properly skipped, including
cleanup from those steps.

Meanwhile, it is _required_ (by make distcheck) that the test clean up
anything it created on a successful run at the end; and this patch
bypasses that which would leave junk behind.  The cleanup at the
beginning is a convenience (useful primarily if the test failed, and you
want to rerun it under a debugger without manually cleaning up first),
but not a necessity.  So NACK to an early exit that bypasses proper cleanup.

> +     * tests/test-canonicalize-lgpl.c (main)[__MINGW32__]: Add basic sanity
> +     checks for mingw along with debug printing to demonstrate brokenness.
> +
> +++ b/tests/test-canonicalize-lgpl.c
> @@ -104,7 +104,41 @@ main (void)
>    }
>  
>  #ifdef __MINGW32__
> +
> +  /* Check basic drive letter sanity.  */
> +  {
> +    char cwd[PATH_MAX];

Stack-allocating PATH_MAX bytes is a bad idea.  True, on mingw, PATH_MAX
is only 256 or so, so it isn't horrible, but it is a bad idiom on almost
every other platform and should be replaced by malloc()ing a proper size.

> +    char *test;
> +    char *result;
> +
> +    /* Check if BASE has a canonical name.  */
> +    result = canonicalize_file_name (BASE);
> +    fprintf (stderr, "BASE-canon: %s\n", result);
> +    ASSERT (result != NULL);
> +
> +    /* Check if BASE's canonical name is somewhat canonical.  */
> +    ASSERT ((strchr (result, '/') == NULL)
> +         != (strchr (result, '\\') == NULL));
> +
> +    /* Check if CWD has a canonical name.  */
> +    getcwd (cwd, PATH_MAX);

Why not rely on gnulib's getcwd() module which guarantees that
getcwd(NULL,0) returns a malloc()d result; then you don't need a
stack-allocated array of PATH_MAX in the first place.

> +    fprintf (stderr, "CWD: %s\n", cwd);
> +    result = canonicalize_file_name (cwd);
> +    fprintf (stderr, "CWD-canon: %s\n", result);
> +    ASSERT (result != NULL);
> +
> +    /* Check basic drive letter sanity.  */
> +    test = "c:/";
> +    result = canonicalize_file_name (test);
> +    ASSERT (strcmp (result, test) == 0);
> +    fprintf (stderr, "C:/-canon: %s\n", result);
> +    result = canonicalize_file_name ("C:\\");
> +    ASSERT (strcmp (result, test) == 0);
> +    fprintf (stderr, "C:\\-canon: %s\n", result);

None of the other unit tests fprintf; it is sufficient to be silent if
the test passes, and use only ASSERT for output if the test failed.

However, other than the improper preprocessor check (rather than #ifdef
__MING32__, you should be using:
#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
), this does indeed look like a reasonable start for a unit test of
canonicalizing behavior on windows-style paths.

> 2011-02-01  Jan Nieuwenhuizen  <address@hidden>
> 
>       * lib/canonicalize-lgpl.c (__realpath)[__MINGW32__]: Add an
>       implementation for canonicalize_file_name.  This marked the first
>       running of guile.exe (1.9) in wine.
> 
>       * tests/test-canonicalize-lgpl.c (main)[__MINGW32__]: Do not abort
>       on `nonexistent/..'; in Windows that works fine.

That's because Windows violates POSIX.  A proper canonicalize
implementation is required to reject that path, which means your first
cut still needs to add some stat() calls.

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

signature.asc
Description: OpenPGP digital signature