On Sun, Mar 8, 2015 at 10:22 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> On 2/24/15 3:06 AM, Michael Paquier wrote:
>> On Sun, Feb 15, 2015 at 11:01 AM, Peter Eisentraut wrote:
>>> Here is an updated patch.
>>
>> Nice patch. This is going to save a lot of resources.
>>
>> An update of vcregress.pl is necessary. This visibly just consists in
>> updating the options that have been renamed in pg_regress (don't mind
>> testing any code sent out).
>
> Well, that turns out to be more complicated than initially thought.
> Apparently, the msvc has a bit of a different idea of what check and
> installcheck do with respect to temporary installs. For instance,
> vcregress installcheck does not use psql from the installation but from
> the build tree. vcregress check uses psql from the build tree but other
> binaries (initdb, pg_ctl) from the temporary installation. It is hard
> for me to straighten this out by just looking at the code. Attached is
> a patch that shows the idea, but I can't easily take it further than that.
Urg. Yes for installcheck I guess that we cannot do much but simply
use the psql from the tree, but on the contrary for all the other
targets we can use the temporary installation as $topdir/tmp_install.
Regarding the patch you sent, IMO it is not a good idea to kick
install with system() as this can fail as an unrecognized command
runnable. And the command that should be used is not "vcregress
install $path" but simply "vcregress install". Hence I think that
calling simply Install() makes more sense. Also, I think that it would
be better to not enforce PATH before kicking the commands.
Speaking of which, attached is a patch rewritten in-line with those
comments, simplifying a bit the whole at the same time. Note this
patch changes ecpgcheck as it should be patched, but as ecpgcheck test
is broken even on HEAD, I'll raise a separate thread about that with a
proper fix (for example bowerbird skips this test).
On my side, with this patch, installcheck, check, plcheck,
upgradecheck work properly and all of them use the common
installation. It would be more adapted to add checks on the existence
of $tmp_installdir/bin though in InstallTemp instead of kicking an
installation all the time. But that's simple enough to change.
Regards,
--
Michael