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

From Bruce Momjian
Subject Re: [HACKERS] pg_regress breaks on msys
Date
Msg-id 200607290226.k6T2Q8h02249@momjian.us
Whole thread Raw
In response to Re: [HACKERS] pg_regress breaks on msys  (Tom Lane <tgl@sss.pgh.pa.us>)
Responses Re: [HACKERS] pg_regress breaks on msys  (Tom Lane <tgl@sss.pgh.pa.us>)
List pgsql-patches
Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> >> Tom Lane wrote:
> >>> The cases that I think we most need to defend against are
> >>> (A) diff program not found
>
> > In summary, on MinGW, files differ or 'diff' not found, returns 1.  If
> > one of the files to be compared does not exist, it returns 2.  And of
> > course, if the files are the same, it returns zero.
>
> OK.  The problem here is that pg_regress is coded to assume that
> zero-length output file represents success.  Given the above Windows
> behavior that is *clearly* not good enough, because that's probably
> exactly what we will see after diff-not-found (if the Windows shell
> acts like a Unix shell does and creates the ">" target first).
>
> I'd suggest modifying the logic so that zero-length output file with a
> nonzero return from the child be treated as a fatal condition (not just
> a difference, but bail out).

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.

--
  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    29 Jul 2006 02:21:57 -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    29 Jul 2006 02:22:23 -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,826 ----
  }

  /*
!  * 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
+     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)
--- 843,849 ----
      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;
      }
--- 871,880 ----
      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;
          }
--- 893,901 ----
          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");
--- 916,922 ----
      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");

pgsql-patches by date:

Previous
From: "Marko Kreen"
Date:
Subject: Re: [HACKERS] [PATCH] Provide 8-byte transaction IDs to user level
Next
From: Joe Conway
Date:
Subject: Re: [HACKERS] 8.2 features?