Hello Peter,
Here is a review:
The version 2 of the patch applies cleanly on current head.
The ability to generate and reuse a temporary installation for different
tests looks quite useful, thus putting install out of pg_regress and in
make seems reasonnable.
However I'm wondering whether there could be some use case of pg_regress
where doing the install might be useful nevertheless, say for manually
doing things on the command line, in which case keeping the feature, even
if not used by default, could be nice?
About changes: I think that more comments would be welcome, eg
with_temp_install.
There is not a single documentation change. Should there be some? Hmmm...
I have not found much documentation about "pg_regress"...
I have tested make check, check-world, as well as make check in contrib
sub-directories. It seems to work fine in sequential mode.
Running "make -j2 check-world" does not work because "initdb" is not found
by "pg_regress". but "make -j1 check-world" does work fine. It seems that
some dependencies might be missing and there is a race condition between
temporary install and running some checks?? Maybe it is not expected to
work anyway? See below suggestions to make it work.
However in this case the error message passed by pg_regress contains an
error:
pg_regress: initdb failed
Examine /home/fabien/DEV/GIT/postgresql/contrib/btree_gin/log/initdb.log for
the reason.
Command was: "initdb" -D "/home/fabien/DEV/GIT/postgresql/contrib/btree_gin/./tmp_check/data"
--noclean --nosync > "/home/fabien/DEV/GIT/postgresql/contrib/btree_gin/./tmp_check/log/initdb.log"
2>&1
make[2]: *** [check] Error 2
The error messages point to non existing log file (./tmp_check is
missing), the temp_instance variable should be used in the error message
as well as in the commands. Maybe other error messages have the same
issues.
I do not like much the MAKELEVEL hack in a phony target. When running in
parallel, several makes may have the same level anyway, not sure what
would happen... Maybe it is the origin of the race condition which makes
initdb not to be found above. I would suggest to try to rely on
dependences, maybe something like the following could work to ensure that
an installation is done once and only once per make invocation, whatever
the underlying parallelism & levels, as well as a possibility to reuse the
previous installation.
# must be defined somewhere common to all makefiles
ifndef MAKE_NONCE
# define a nanosecond timestamp
export MAKE_NONCE := $(shell date +%s.%N)
endif
# actual new tmp installation
.tmp_install:
$(RM) ./.tmp_install.*
$(RM) -r ./tmp_install
# create tmp installation...
touch $@
# tmp installation for the nonce
.tmp_install.$(MAKE_NONCE): .tmp_install
touch $@
# generate a new tmp installation by default
# "make USE_TMP_INSTALL=1 ..." reuses a previous installation if available
ifdef USE_TMP_INSTALL
TMP_INSTALL = .tmp_install
else
TMP_INSTALL = .tmp_install.$(MAKE_NONCE)
endif # USE_TMP_INSTALL
.PHONY: main-temp-install
main-temp-install: $(TMP_INSTALL)
.PHONY: extra-temp-install
extra-temp-install: main-temp-install
# do EXTRA_INSTALL
MSVC build is not yet adjusted for this. Looking at vcregress.pl, this
should be fairly straightforward.
No idea about that point.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
