regress bug
I have just found, after an hour or two of frustration, that pg_regress'
--inputdir and --outputdir options don't play nicely with the
convert_sourcefiles routines. In effect these options are ignored as
destinations for converted files, and the destination for converted
files is apparently always $CWD/{sql,expected}. The upshot is that these
options are pretty much unusable if you want to have converted files
(which would, for example, specify the location of data files).
This seems like an outright bug. I don't recall any discussion on it.
Maybe nobody's come across it before. ISTM the correct behaviour would
be to put converted sql files in $inputdir/sql and converted results
files in $outputdir/expected.
Thoughts?
cheers
andrew
On Mar 8, 2012, at 10:22 AM, Andrew Dunstan wrote:
This seems like an outright bug. I don't recall any discussion on it. Maybe nobody's come across it before. ISTM the correct behaviour would be to put converted sql files in $inputdir/sql and converted results files in $outputdir/expected.
In my extension distributions, I have
tests/sql
tests/expected
And for that, --inputdir=test works just fine. I don't mess with --outputdir, which just seems to affect where the actual output is written to, which is just a directory named regression.out at the top of the project.
Best,
David
On 03/08/2012 02:59 PM, David E. Wheeler wrote:
On Mar 8, 2012, at 10:22 AM, Andrew Dunstan wrote:
This seems like an outright bug. I don't recall any discussion on it. Maybe nobody's come across it before. ISTM the correct behaviour would be to put converted sql files in $inputdir/sql and converted results files in $outputdir/expected.
In my extension distributions, I have
tests/sql
tests/expectedAnd for that, --inputdir=test works just fine. I don't mess with --outputdir, which just seems to affect where the actual output is written to, which is just a directory named regression.out at the top of the project.
It works fine if you don't need to do any file conversions (i.e. if you
don't have "input" or "output" directories). But file_textarray_fdw does.
Here's a patch that I think fixes the problem.
cheers
andrew
Attachments:
regress.patchtext/x-patch; name=regress.patchDownload
*** a/src/test/regress/pg_regress.c
--- b/src/test/regress/pg_regress.c
***************
*** 407,413 **** replace_string(char *string, char *replace, char *replacement)
* the given suffix.
*/
static void
! convert_sourcefiles_in(char *source_subdir, char *dest_subdir, char *suffix)
{
char testtablespace[MAXPGPATH];
char indir[MAXPGPATH];
--- 407,413 ----
* the given suffix.
*/
static void
! convert_sourcefiles_in(char *source_subdir, char *dest_dir, char *dest_subdir, char *suffix)
{
char testtablespace[MAXPGPATH];
char indir[MAXPGPATH];
***************
*** 475,481 **** convert_sourcefiles_in(char *source_subdir, char *dest_subdir, char *suffix)
/* 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_subdir, prefix, suffix);
infile = fopen(srcfile, "r");
if (!infile)
--- 475,482 ----
/* 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.%s", dest_dir, dest_subdir,
! prefix, suffix);
infile = fopen(srcfile, "r");
if (!infile)
***************
*** 522,529 **** convert_sourcefiles_in(char *source_subdir, char *dest_subdir, char *suffix)
static void
convert_sourcefiles(void)
{
! convert_sourcefiles_in("input", "sql", "sql");
! convert_sourcefiles_in("output", "expected", "out");
}
/*
--- 523,530 ----
static void
convert_sourcefiles(void)
{
! convert_sourcefiles_in("input", inputdir, "sql", "sql");
! convert_sourcefiles_in("output", outputdir, "expected", "out");
}
/*
On Mar 8, 2012, at 12:20 PM, Andrew Dunstan wrote:
It works fine if you don't need to do any file conversions (i.e. if you don't have "input" or "output" directories). But file_textarray_fdw does.
Here's a patch that I think fixes the problem.
While you’re there, an issue I noticed is that the MODULE_PATHNAME substitutions do not work if you have your SQL files in a subdirectory. My extensions have the .sql files in an sql/ directory. So that means when I have something like this in sql/plproxy.sql.in:
CREATE FUNCTION plproxy_call_handler ()
RETURNS language_handler AS 'MODULE_PATHNAME' LANGUAGE C;
What I end up with in sql/plproxy.sql is:
CREATE FUNCTION plproxy_call_handler ()
RETURNS language_handler AS 'sql/plproxy' LANGUAGE C;
Which doesn’t work at all, because the file is not installed in an `sql` subdirectory, it's just that way in my repository (and the distribution tarball). So I avoid the whole MODULE_PATHNAME business for now (and the .in file) and just do this, instead:
CREATE FUNCTION plproxy_call_handler ()
RETURNS language_handler AS 'plproxy' LANGUAGE C;
Which is an okay workaround, but I’m not sure that MODULE_PATHNAME is actually working correctly.
Best,
David
"David E. Wheeler" <david@justatheory.com> writes:
While you�re there, an issue I noticed is that the MODULE_PATHNAME
substitutions do not work if you have your SQL files in a
subdirectory.
Huh? MODULE_PATHNAME is not substituted by pg_regress at all (anymore
anyway). There's still some vestigial support for it in pgxs.mk, but
the future of that code is to vanish, not get improved. You should
not be needing it to get substituted at build time either.
regards, tom lane
On Mar 8, 2012, at 12:59 PM, Tom Lane wrote:
Huh? MODULE_PATHNAME is not substituted by pg_regress at all (anymore
anyway).
Yeah, sorry, I meant `make`.
There's still some vestigial support for it in pgxs.mk, but
the future of that code is to vanish, not get improved. You should
not be needing it to get substituted at build time either.
I still see this pattern a *lot*; I removed it from PL/Proxy last week. The attached tarball demonstrates it:
make
sed 's,MODULE_PATHNAME,$libdir/sql/exttest,g' sql/exttest.sql.in >sql/exttest.sql
make: *** No rule to make target `exttest.so', needed by `all'. Stop.
So MODULE_PATHNAME is replaced with $libdir/sql/exttest rather than $libdir/exttest. Maybe that should not be fixed, but there are a *lot* of extensions out there using this approach (copied from contrib, which used it for years, albeit without the .sql.in files in a subdirectory).
So perhaps DATA_built is to be removed from pgxs.mk? And if so, is the idea then that one should just put the module name in the .sql file, rather than MODULE_PATHNAME in a .sql.in file?
Best,
David
Attachments:
On 03/08/2012 04:33 PM, David E. Wheeler wrote:
On Mar 8, 2012, at 12:59 PM, Tom Lane wrote:
Huh? MODULE_PATHNAME is not substituted by pg_regress at all (anymore
anyway).Yeah, sorry, I meant `make`.
There's still some vestigial support for it in pgxs.mk, but
the future of that code is to vanish, not get improved. You should
not be needing it to get substituted at build time either.I still see this pattern a *lot*; I removed it from PL/Proxy last week. The attached tarball demonstrates it:
make
sed 's,MODULE_PATHNAME,$libdir/sql/exttest,g' sql/exttest.sql.in>sql/exttest.sql
make: *** No rule to make target `exttest.so', needed by `all'. Stop.So MODULE_PATHNAME is replaced with $libdir/sql/exttest rather than $libdir/exttest. Maybe that should not be fixed, but there are a *lot* of extensions out there using this approach (copied from contrib, which used it for years, albeit without the .sql.in files in a subdirectory).
So perhaps DATA_built is to be removed from pgxs.mk? And if so, is the idea then that one should just put the module name in the .sql file, rather than MODULE_PATHNAME in a .sql.in file?
Extensions (unlike non-extension modules) should not monkey with
MODULE_PATHNAME at all.
Change the Makefile def from DATA_built to DATA and rename the file to
exttest.sql
cheers
andrew
On Mar 8, 2012, at 1:45 PM, Andrew Dunstan wrote:
So perhaps DATA_built is to be removed from pgxs.mk? And if so, is the idea then that one should just put the module name in the .sql file, rather than MODULE_PATHNAME in a .sql.in file?
Extensions (unlike non-extension modules) should not monkey with MODULE_PATHNAME at all.
Change the Makefile def from DATA_built to DATA and rename the file to exttest.sql
I did (and I do). But this is not documented or recommended anywhere. Something should be promulgated to encourage existing authors to do that.
David