Thread: pg_regress: stat correct paths

Hi,

I think I have found a small problem in pg_regress.

In convert_sourcefiles() it stats directories based on the current
working directory, but in convert_sourcefiles_in() it reads files based
in srcdir or abs_builddir.

The attached patch changes the behavior of pg_regress, so that it stats
the directories that it read the files from. This patch will also make
pg_regress fail if the directories are missing, instead of silently
ignoring the missing directories. If you think this fail-fast behavior
is undesirable, I can create a patch that just ignores the files (todays
behavior), or ignores them with a warning.

The attached patch is tested by running pg_regress in a non-standard
path and by running "make check", both on Solaris Nevada on x64.

-J
--

Jørgen Austvik, Software Engineering - QA
Sun Microsystems Database Technology Group

Index: src/test/regress/pg_regress.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/test/regress/pg_regress.c,v
retrieving revision 1.38
diff -c -r1.38 pg_regress.c
*** src/test/regress/pg_regress.c    15 Nov 2007 21:14:46 -0000    1.38
--- src/test/regress/pg_regress.c    26 Nov 2007 14:26:54 -0000
***************
*** 399,407 ****
--- 399,410 ----
      char        abs_builddir[MAXPGPATH];
      char        testtablespace[MAXPGPATH];
      char        indir[MAXPGPATH];
+     char        destdir[MAXPGPATH];
      char      **name;
      char      **names;
      int            count = 0;
+     struct stat    st;
+     int        ret;

  #ifdef WIN32
      char       *c;
***************
*** 424,429 ****
--- 427,450 ----
          strcpy(abs_srcdir, abs_builddir);

      snprintf(indir, MAXPGPATH, "%s/%s", abs_srcdir, source);
+     snprintf(destdir, MAXPGPATH, "%s/%s", abs_srcdir, dest);
+
+     ret = stat(indir, &st);
+     if ((ret != 0) || (!S_ISDIR(st.st_mode)))
+     {
+         fprintf(stderr, _("%s: could not find input directory \"%s\": %s\n"),
+                 progname, indir, strerror(errno));
+         exit_nicely(2);
+     }
+
+     ret = stat(destdir, &st);
+     if ((ret != 0) || (!S_ISDIR(st.st_mode)))
+     {
+         fprintf(stderr, _("%s: could not find destination directory \"%s\": %s\n"),
+                 progname, destdir, strerror(errno));
+         exit_nicely(2);
+     }
+
      names = pgfnames(indir);
      if (!names)
          /* Error logged in pgfnames */
***************
*** 466,472 ****
          /* build the full actual paths to open */
          snprintf(prefix, strlen(*name) - 6, "%s", *name);
          snprintf(srcfile, MAXPGPATH, "%s/%s", indir, *name);
!         snprintf(destfile, MAXPGPATH, "%s/%s.%s", dest, prefix, suffix);

          infile = fopen(srcfile, "r");
          if (!infile)
--- 487,493 ----
          /* build the full actual paths to open */
          snprintf(prefix, strlen(*name) - 6, "%s", *name);
          snprintf(srcfile, MAXPGPATH, "%s/%s", indir, *name);
!         snprintf(destfile, MAXPGPATH, "%s/%s.%s", destdir, prefix, suffix);

          infile = fopen(srcfile, "r");
          if (!infile)
***************
*** 508,527 ****
      pgfnames_cleanup(names);
  }

! /* Create the .sql and .out files from the .source files, if any */
  static void
  convert_sourcefiles(void)
  {
!     struct stat st;
!     int            ret;
!
!     ret = stat("input", &st);
!     if (ret == 0 && S_ISDIR(st.st_mode))
!         convert_sourcefiles_in("input", "sql", "sql");
!
!     ret = stat("output", &st);
!     if (ret == 0 && S_ISDIR(st.st_mode))
!         convert_sourcefiles_in("output", "expected", "out");
  }

  /*
--- 529,540 ----
      pgfnames_cleanup(names);
  }

! /* Create the .sql and .out files from the .source files */
  static void
  convert_sourcefiles(void)
  {
!     convert_sourcefiles_in("input", "sql", "sql");
!     convert_sourcefiles_in("output", "expected", "out");
  }

  /*

Jorgen Austvik - Sun Norway wrote:

> This patch will also make pg_regress fail if the directories are
> missing, instead of silently ignoring the missing directories. If you
> think this fail-fast behavior is undesirable, I can create a patch
> that just ignores the files (todays behavior), or ignores them with a
> warning.

This silent-skip behavior is there to avoid noise in other tests.  Try
the ecpg tests, contrib tests and src/pl tests.

I didn't check the rest of the patch, but did you verify that it still
works in VPATH builds?

--
Alvaro Herrera                        http://www.advogato.org/person/alvherre
"Nunca confiaré en un traidor.  Ni siquiera si el traidor lo he creado yo"
(Barón Vladimir Harkonnen)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I didn't check the rest of the patch, but did you verify that it still
> works in VPATH builds?

Actually, it looks to me like the patch is wrong specifically because
it does not do the right thing in the VPATH case.

            regards, tom lane

Alvaro Herrera wrote:
> Jorgen Austvik - Sun Norway wrote:
>
>> This patch will also make pg_regress fail if the directories are
>> missing, instead of silently ignoring the missing directories. If you
>> think this fail-fast behavior is undesirable, I can create a patch
>> that just ignores the files (todays behavior), or ignores them with a
>> warning.
>
> This silent-skip behavior is there to avoid noise in other tests.  Try
> the ecpg tests, contrib tests and src/pl tests.

OK, thank you for the pointers, I will look into this.

I will send out a new patch that is tested with those, and that does the
silent-skip.

> I didn't check the rest of the patch, but did you verify that it still
> works in VPATH builds?

I saw two comments in the file mentioning VPATH, but I have no idea
about what that is. Based on reading the code, I belive this patch
should work better with VPATH (setting srcpath) than without, as it
actually checks those folders instead of other folders.

-J
--
Jørgen Austvik, Software Engineering
Database Technology Group

Tom Lane wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>> I didn't check the rest of the patch, but did you verify that it still
>> works in VPATH builds?
>
> Actually, it looks to me like the patch is wrong specifically because
> it does not do the right thing in the VPATH case.

Are you thinking about "failing if the folders are missing" as "not the
right thing in the VPATH case", or are you thinking about something else?

-J
--
Jørgen Austvik, Software Engineering
Database Technology Group

Jorgen Austvik <Jorgen.Austvik@Sun.COM> writes:
> Tom Lane wrote:
>> Actually, it looks to me like the patch is wrong specifically because
>> it does not do the right thing in the VPATH case.

> Are you thinking about "failing if the folders are missing" as "not the
> right thing in the VPATH case", or are you thinking about something else?

The point is that in VPATH you are running in a build tree, and should
copy the source files from the source tree, but *not* modify the source
tree.  Thus, fetching relative to $srcdir but writing relative to .
is in fact the correct behavior.

There has not previously been any complaint that pg_regress was broken
in this regard, so maybe you should take two steps back and explain what
problem you think needs fixing, rather than just dropping a patch on us.

            regards, tom lane

Tom Lane wrote:
> Jorgen Austvik <Jorgen.Austvik@Sun.COM> writes:
>> Tom Lane wrote:
>>> Actually, it looks to me like the patch is wrong specifically because
>>> it does not do the right thing in the VPATH case.
>
>> Are you thinking about "failing if the folders are missing" as "not the
>> right thing in the VPATH case", or are you thinking about something else?
>
> The point is that in VPATH you are running in a build tree, and should
> copy the source files from the source tree, but *not* modify the source
> tree.  Thus, fetching relative to $srcdir but writing relative to .
> is in fact the correct behavior.

Ah, I understand.

It is this part you don't like:
---8<--------------8<--------------8<--------------8<--------------8<-----------
+     snprintf(destdir, MAXPGPATH, "%s/%s", abs_srcdir, dest);
<snip>
!         snprintf(destfile, MAXPGPATH, "%s/%s.%s", destdir, prefix, suffix);
---8<--------------8<--------------8<--------------8<--------------8<-----------

Thanks for the guidance, I'll try come up with a better alternative.
Should outputdir be used instead of current working directory (cwd) if
it is set?

> There has not previously been any complaint that pg_regress was broken
> in this regard, so maybe you should take two steps back and explain what
> problem you think needs fixing, rather than just dropping a patch on us.

I tried to explain it in the mail, but let me try again, this time
showing some code.

Here we stat <cwd>/input:

---8<--------------8<--------------8<--------------8<--------------8<-----------
static void
convert_sourcefiles(void)
{
    struct stat st;
    int            ret;

    ret = stat("input", &st);
    if (ret == 0 && S_ISDIR(st.st_mode))
        convert_sourcefiles_in("input", "sql", "sql");
---8<--------------8<--------------8<--------------8<--------------8<-----------

But if we have set srcdir, the directory we are stat'ing, is not the
same directory that we are reading the files from:

---8<--------------8<--------------8<--------------8<--------------8<-----------
static void
convert_sourcefiles_in(char *source, char *dest, char *suffix)

         <snip>

    if (srcdir)
        strcpy(abs_srcdir, srcdir);
    else
        strcpy(abs_srcdir, abs_builddir);

    snprintf(indir, MAXPGPATH, "%s/%s", abs_srcdir, source);
    names = pgfnames(indir);
---8<--------------8<--------------8<--------------8<--------------8<-----------

So I wanted to provide a patch that ran stat on the folder that we were
reading the files from.

-J
--
Jørgen Austvik, Software Engineering
Database Technology Group

Jorgen Austvik <Jorgen.Austvik@Sun.COM> writes:
> But if we have set srcdir, the directory we are stat'ing, is not the
> same directory that we are reading the files from:

Ah.  The reason this works in VPATH mode is that setup of the build tree
duplicated all subdirectories of the source tree, so ./input/ should
exist iff $srcdir/input/ does.  I agree it's a bit ugly though; it'd
be better to stat what we really plan to read.

Maybe push the stat operation inside convert_sourcefiles_in ?

            regards, tom lane

Tom Lane wrote:
> Ah.  The reason this works in VPATH mode is that setup of the build tree
> duplicated all subdirectories of the source tree, so ./input/ should
> exist iff $srcdir/input/ does.  I agree it's a bit ugly though; it'd
> be better to stat what we really plan to read.
>
> Maybe push the stat operation inside convert_sourcefiles_in ?

Yes, that was what I tried to do in the patch, but unfortunately I was
too eager :) I'll send a new proposal tomorrow. Thanks for your help!

-J
--
Jørgen Austvik, Software Engineering
Database Technology Group

Jorgen Austvik wrote:
>> Maybe push the stat operation inside convert_sourcefiles_in ?
>
> Yes, that was what I tried to do in the patch, but unfortunately I was
> too eager :) I'll send a new proposal tomorrow. Thanks for your help!

Hi,

this patch should only move the stat operation into
convert_sourcefiles_in().

The attached patch is tested by running pg_regress in a non-standard
path, and by running "make check" in pgsql plus in ecpg.

-J
--

Jørgen Austvik, Software Engineering - QA
Database Technology Group
Index: src/test/regress/pg_regress.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/test/regress/pg_regress.c,v
retrieving revision 1.38
diff -c -r1.38 pg_regress.c
*** src/test/regress/pg_regress.c    15 Nov 2007 21:14:46 -0000    1.38
--- src/test/regress/pg_regress.c    27 Nov 2007 14:13:47 -0000
***************
*** 399,404 ****
--- 399,406 ----
      char        abs_builddir[MAXPGPATH];
      char        testtablespace[MAXPGPATH];
      char        indir[MAXPGPATH];
+     struct stat    st;
+     int        ret;
      char      **name;
      char      **names;
      int            count = 0;
***************
*** 424,429 ****
--- 426,440 ----
          strcpy(abs_srcdir, abs_builddir);

      snprintf(indir, MAXPGPATH, "%s/%s", abs_srcdir, source);
+
+     ret = stat(indir, &st);
+     if ((ret != 0) || (!S_ISDIR(st.st_mode)))
+         /*
+          * No warning, to avoid noise in tests that does not have
+          * these directories, see ecpg, contrib and src/pl.
+          */
+         return;
+
      names = pgfnames(indir);
      if (!names)
          /* Error logged in pgfnames */
***************
*** 512,527 ****
  static void
  convert_sourcefiles(void)
  {
!     struct stat st;
!     int            ret;
!
!     ret = stat("input", &st);
!     if (ret == 0 && S_ISDIR(st.st_mode))
!         convert_sourcefiles_in("input", "sql", "sql");
!
!     ret = stat("output", &st);
!     if (ret == 0 && S_ISDIR(st.st_mode))
!         convert_sourcefiles_in("output", "expected", "out");
  }

  /*
--- 523,530 ----
  static void
  convert_sourcefiles(void)
  {
!     convert_sourcefiles_in("input", "sql", "sql");
!     convert_sourcefiles_in("output", "expected", "out");
  }

  /*

Jorgen Austvik - Sun Norway <Jorgen.Austvik@Sun.COM> writes:
> this patch should only move the stat operation into
> convert_sourcefiles_in().

Applied, thanks.

            regards, tom lane