Date: Sun, 31 Aug 2014 11:36:36 +0200 (CEST)
From: Fabien COELHO <coelho@cri.ensmp.fr>
To: Peter Eisentraut <peter_e@gmx.net>
cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: improving speed of make check-world
In-Reply-To: <53ED9E6C.1080700@gmx.net>
Message-ID: <alpine.DEB.2.10.1408311125080.14824@sto>
References: <53ED9E6C.1080700@gmx.net>
User-Agent: Alpine 2.10 (DEB 1266 2009-07-14)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


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