pg_regress: stat correct paths

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 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

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

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

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