Thread: Re: [HACKERS] pg_regress breaks on msys
Tom Lane wrote: > "Hiroshi Saito" <z-saito@guitar.ocn.ne.jp> writes: > > This is very strange.!! > > boolean ... ok > > char ... diff command failed with status 1: "diff -w "./expected/char.out" > > "./results/char.out" >"./results/char.diff"" > > server stopped > > Yes, I believe the problem is that our Windows versions of the > WIFEXITED/WEXITSTATUS macros are wrong. pg_regress is trying to verify > that the diff call didn't fail entirely (eg, diff not there or failed > to read one of the files), but the status code diff is returning for > "files not the same" is confusing it. > > Can anyone check into what the result status conventions really are on > Windows? I am tempted to change the macros to just swap the bytes, > but I dunno what that will do to their existing usages to check the > result of pclose() or win32_waitpid(). I checked on MinGW and system() just returns the value returned by the application. There isn't any special two-values-in-one layering like is done on Unix for wait() and the return value from system(). It seems if the child dies from a signal, the parent dies too, at least in my C tests. I propose the following patch which just makes Win32 behave as returning a single value from system(), and cleans up pg_regress.c. We don't call wait() or pipe() on Win32. Though this is clearly a bug fix, I am hesitant to backpatch it to 8.1 or 8.0 because of no problem reports. -- 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 28 Jul 2006 19:01:07 -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 28 Jul 2006 19:01:12 -0000 *************** *** 813,823 **** * 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); --- 813,819 ----
Bruce Momjian <bruce@momjian.us> writes:
> *** src/test/regress/pg_regress.c 27 Jul 2006 15:37:19 -0000 1.16
> --- src/test/regress/pg_regress.c 28 Jul 2006 19:01:12 -0000
> ***************
> *** 813,823 ****
> * 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);
Um, please also remove the comment just above that.
regards, tom lane
Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > *** src/test/regress/pg_regress.c 27 Jul 2006 15:37:19 -0000 1.16
> > --- src/test/regress/pg_regress.c 28 Jul 2006 19:01:12 -0000
> > ***************
> > *** 813,823 ****
> > * 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);
>
> Um, please also remove the comment just above that.
Done.
--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes:
> I checked on MinGW and system() just returns the value returned by the
> application. There isn't any special two-values-in-one layering like is
> done on Unix for wait() and the return value from system(). It seems if
> the child dies from a signal, the parent dies too, at least in my C
> tests.
The cases that I think we most need to defend against are
(A) diff program not found
(B) diff fails to read one of the input files
I think your proposal handles case B, because diff should return exit
code 2 which we will detect, but what happens in case A? Please test it.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > I checked on MinGW and system() just returns the value returned by the > > application. There isn't any special two-values-in-one layering like is > > done on Unix for wait() and the return value from system(). It seems if > > the child dies from a signal, the parent dies too, at least in my C > > tests. > > The cases that I think we most need to defend against are > > (A) diff program not found > > (B) diff fails to read one of the input files > > I think your proposal handles case B, because diff should return exit > code 2 which we will detect, but what happens in case A? Please test it. It returns 1. -- Bruce Momjian bruce@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Tom Lane wrote: > > Bruce Momjian <bruce@momjian.us> writes: > > > I checked on MinGW and system() just returns the value returned by the > > > application. There isn't any special two-values-in-one layering like is > > > done on Unix for wait() and the return value from system(). It seems if > > > the child dies from a signal, the parent dies too, at least in my C > > > tests. > > > > The cases that I think we most need to defend against are > > > > (A) diff program not found > > > > (B) diff fails to read one of the input files > > > > I think your proposal handles case B, because diff should return exit > > code 2 which we will detect, but what happens in case A? Please test it. > > It returns 1. 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. I assume MSVC builds will have problem with the diff call. -- Bruce Momjian bruce@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
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).
regards, tom lane
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");
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)
...
regards, tom lane
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");