Tom Lane wrote:
Bruce Momjian [EMAIL PROTECTED] 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 [EMAIL PROTECTED]
EnterpriseDBhttp://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 - 1.53
--- src/include/port/win32.h 29 Jul 2006 02:21:57 -
***
*** 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 - 1.16
--- src/test/regress/pg_regress.c 29 Jul 2006 02:22:23 -
***
*** 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