Re: [HACKERS] pg_regress breaks on msys - Mailing list pgsql-patches

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > I modified pg_regress.c to use just the return code to determine if the
> > diff worked, but I added in a WIN32-specific test for the file size.  I
> > think that is the cleanest solution.  Attached.
>
> It really needs a comment, along the lines of
>
>     /*
>      * On Windows we'll get exit status 1 if the diff invocation
>      * failed; so we need a way to distinguish failure from "files
>      * are different".  Check to make sure that a diff file was
>      * created and is nonempty.
>      */
>
> Also the test ought to account for file_size returning -1 if file's
> not there, so
>
> + #ifdef WIN32
> +     if (WEXITSTATUS(r) == 1 && file_size(filename) <= 0)
> ...

I had already added a comment this morning, but didn't post the updated
patch.  I have made the adjustment for -1.

Patch attached and applied.

--
  Bruce Momjian   bruce@momjian.us
  EnterpriseDB    http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +
Index: src/include/port/win32.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/port/win32.h,v
retrieving revision 1.53
diff -c -c -r1.53 win32.h
*** src/include/port/win32.h    16 Jul 2006 20:17:04 -0000    1.53
--- src/include/port/win32.h    30 Jul 2006 01:41:46 -0000
***************
*** 109,120 ****


  /*
!  * Signal stuff
   */
! #define WEXITSTATUS(w)    (((w) >> 8) & 0xff)
! #define WIFEXITED(w)    (((w) & 0xff) == 0)
! #define WIFSIGNALED(w)    (((w) & 0x7f) > 0 && (((w) & 0x7f) < 0x7f))
! #define WTERMSIG(w)        ((w) & 0x7f)

  #define sigmask(sig) ( 1 << ((sig)-1) )

--- 109,125 ----


  /*
!  *    Signal stuff
!  *    WIN32 doesn't have wait(), so the return value for children
!  *    is simply the return value specified by the child, without
!  *    any additional information on whether the child terminated
!  *    on its own or via a signal.  These macros are also used
!  *    to interpret the return value of system().
   */
! #define WEXITSTATUS(w)    (w)
! #define WIFEXITED(w)    (true)
! #define WIFSIGNALED(w)    (false)
! #define WTERMSIG(w)        (0)

  #define sigmask(sig) ( 1 << ((sig)-1) )

Index: src/test/regress/pg_regress.c
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/pg_regress.c,v
retrieving revision 1.16
diff -c -c -r1.16 pg_regress.c
*** src/test/regress/pg_regress.c    27 Jul 2006 15:37:19 -0000    1.16
--- src/test/regress/pg_regress.c    30 Jul 2006 01:41:47 -0000
***************
*** 799,827 ****
  }

  /*
!  * Run a "diff" command and check that it didn't crash
   */
! static void
! run_diff(const char *cmd)
  {
      int r;

      r = system(cmd);
-     /*
-      * XXX FIXME: it appears that include/port/win32.h's definitions of
-      * WIFEXITED and related macros may be wrong.  They certainly don't
-      * work for inspecting the results of system().  For the moment,
-      * hard-wire the check on Windows.
-      */
- #ifndef WIN32
      if (!WIFEXITED(r) || WEXITSTATUS(r) > 1)
- #else
-     if (r != 0 && r != 1)
- #endif
      {
          fprintf(stderr, _("diff command failed with status %d: %s\n"), r, cmd);
          exit_nicely(2);
      }
  }

  /*
--- 799,830 ----
  }

  /*
!  * Run a "diff" command and also check that it didn't crash
   */
! static int
! run_diff(const char *cmd, const char *filename)
  {
      int r;

      r = system(cmd);
      if (!WIFEXITED(r) || WEXITSTATUS(r) > 1)
      {
          fprintf(stderr, _("diff command failed with status %d: %s\n"), r, cmd);
          exit_nicely(2);
      }
+ #ifdef WIN32
+     /*
+      *    On WIN32, if the 'diff' command cannot be found, system() returns
+      *    1, but produces nothing to stdout, so we check for that here.
+      */
+     if (WEXITSTATUS(r) == 1 && file_size(filename) <= 0)
+     {
+         fprintf(stderr, _("diff command not found: %s\n"), cmd);
+         exit_nicely(2);
+     }
+ #endif
+
+     return WEXITSTATUS(r);
  }

  /*
***************
*** 844,850 ****
      int best_line_count;
      int i;
      int l;
!
      /* Check in resultmap if we should be looking at a different file */
      expectname = testname;
      for (rm = resultmap; rm != NULL; rm = rm->next)
--- 847,853 ----
      int best_line_count;
      int i;
      int l;
!
      /* Check in resultmap if we should be looking at a different file */
      expectname = testname;
      for (rm = resultmap; rm != NULL; rm = rm->next)
***************
*** 872,883 ****
      snprintf(cmd, sizeof(cmd),
               SYSTEMQUOTE "diff %s \"%s\" \"%s\" > \"%s\"" SYSTEMQUOTE,
               basic_diff_opts, expectfile, resultsfile, diff);
-     run_diff(cmd);

      /* Is the diff file empty? */
!     if (file_size(diff) == 0)
      {
-         /* No diff = no changes = good */
          unlink(diff);
          return false;
      }
--- 875,884 ----
      snprintf(cmd, sizeof(cmd),
               SYSTEMQUOTE "diff %s \"%s\" \"%s\" > \"%s\"" SYSTEMQUOTE,
               basic_diff_opts, expectfile, resultsfile, diff);

      /* Is the diff file empty? */
!     if (run_diff(cmd, diff) == 0)
      {
          unlink(diff);
          return false;
      }
***************
*** 896,906 ****
          snprintf(cmd, sizeof(cmd),
                   SYSTEMQUOTE "diff %s \"%s\" \"%s\" > \"%s\"" SYSTEMQUOTE,
                   basic_diff_opts, expectfile, resultsfile, diff);
-         run_diff(cmd);

!         if (file_size(diff) == 0)
          {
-             /* No diff = no changes = good */
              unlink(diff);
              return false;
          }
--- 897,905 ----
          snprintf(cmd, sizeof(cmd),
                   SYSTEMQUOTE "diff %s \"%s\" \"%s\" > \"%s\"" SYSTEMQUOTE,
                   basic_diff_opts, expectfile, resultsfile, diff);

!         if (run_diff(cmd, diff) == 0)
          {
              unlink(diff);
              return false;
          }
***************
*** 921,927 ****
      snprintf(cmd, sizeof(cmd),
               SYSTEMQUOTE "diff %s \"%s\" \"%s\" >> \"%s\"" SYSTEMQUOTE,
               pretty_diff_opts, best_expect_file, resultsfile, difffilename);
!     run_diff(cmd);

      /* And append a separator */
      difffile = fopen(difffilename, "a");
--- 920,926 ----
      snprintf(cmd, sizeof(cmd),
               SYSTEMQUOTE "diff %s \"%s\" \"%s\" >> \"%s\"" SYSTEMQUOTE,
               pretty_diff_opts, best_expect_file, resultsfile, difffilename);
!     run_diff(cmd, difffilename);

      /* And append a separator */
      difffile = fopen(difffilename, "a");