Thread: contribcheck and modulescheck of MSVC's vcregress.pl cannot work independently

Hi all,

I have just noticed that the commands contribcheck and modulescheck
will fail as follows if the temporary installation is not done in
$ROOT_DIR/tmp_install first:
C:\Users\ioltas\git\postgres\src\tools\msvc>vcregress contribcheck
============================================================
Checking btree_gin
The system cannot find the path specified.

All the other commands call InstallTemp() so as to have them work
independently. For consistency I think that we should do the same for
them. Attached is a patch to fix that.
Regards,
--
Michael

On 06/01/2015 01:49 AM, Michael Paquier wrote:
> Hi all,
>
> I have just noticed that the commands contribcheck and modulescheck
> will fail as follows if the temporary installation is not done in
> $ROOT_DIR/tmp_install first:
> C:\Users\ioltas\git\postgres\src\tools\msvc>vcregress contribcheck
> ============================================================
> Checking btree_gin
> The system cannot find the path specified.
>
> All the other commands call InstallTemp() so as to have them work
> independently. For consistency I think that we should do the same for
> them. Attached is a patch to fix that.


Seems reasonable. I'll take a look.

cheers

andrew

On Sun, May 31, 2015 at 10:49:46PM -0700, Michael Paquier wrote:
> I have just noticed that the commands contribcheck and modulescheck
> will fail as follows if the temporary installation is not done in
> $ROOT_DIR/tmp_install first:
> C:\Users\ioltas\git\postgres\src\tools\msvc>vcregress contribcheck
> ============================================================
> Checking btree_gin
> The system cannot find the path specified.
>
> All the other commands call InstallTemp() so as to have them work
> independently. For consistency I think that we should do the same for
> them. Attached is a patch to fix that.

> --- a/src/tools/msvc/vcregress.pl
> +++ b/src/tools/msvc/vcregress.pl
> @@ -290,6 +290,7 @@ sub subdircheck
>
>  sub contribcheck
>  {
> +    InstallTemp();

This worked around defects in commit dcae5fa: "check", "ecpgcheck" and
"upgradecheck" are the only test targets properly requiring an installation.
The others are installcheck-style targets that need just a couple of binaries
from the build tree; they should be using --bindir=<relpath>/$Config/psql like
installcheck itself.

On Sat, Jul 4, 2015 at 10:37 AM, Noah Misch wrote:
> This worked around defects in commit dcae5fa: "check", "ecpgcheck" and
> "upgradecheck" are the only test targets properly requiring an installation.
> The others are installcheck-style targets that need just a couple of binaries
> from the build tree; they should be using --bindir=<relpath>/$Config/psql like
> installcheck itself.

Well, I disagree here. For one, that's a cheap insurance regarding the
fact that a test set may need more than psql as a binary and it makes
all the tests use the same consistent way of doing. Also, if we would
want to have a real installcheck mode, what we should use is not the
path to what has been built but the path to the installation that the
Postgres instance needed is using. Now if you want to fix it if you
fix that's incorrect I won't complain about that :)

Hence, perhaps that a complete answer is to actually gulp completely the bullet:
- Make installcheck look for pg_config in PATH, and use it to
determine where are the builds. Give as well the option for user to
define a custom path at will.
- Switch all the targets to create by themselves a Postgres instance
similarly to the normal Make (I have always found troublesome that it
is necessary to install contrib/ when wanting to test them, and to
start a Postgres service).
- Create an allcheck target, that will kick all the targets, except
installcheck.
- And an allinstallcheck target that will use an existing instance for
all the targets, with installcheck.
If we can give up on installcheck, I think that we should as well be
able to pass an option "use the existing instance" using an option in
vcregress.pl.
I don't think that this is more than a couple of hours of work, and
will be happy to send a patch for 9.6 if people like those ideas.
Regards,
--
Michael

On Sat, Jul 04, 2015 at 11:26:07AM +0900, Michael Paquier wrote:
> On Sat, Jul 4, 2015 at 10:37 AM, Noah Misch wrote:
> > This worked around defects in commit dcae5fa: "check", "ecpgcheck" and
> > "upgradecheck" are the only test targets properly requiring an installation.
> > The others are installcheck-style targets that need just a couple of binaries
> > from the build tree; they should be using --bindir=<relpath>/$Config/psql like
> > installcheck itself.
>
> Well, I disagree here. For one, that's a cheap insurance regarding the
> fact that a test set may need more than psql as a binary and it makes
> all the tests use the same consistent way of doing.

At several seconds and >1000 files created, an extra temporary installation is
not cheap.  (Indeed, that expense motivated commit dcae5fa.)  The GNU make
build system never creates a temporary installation for just psql.  Insurance
is not valuable in this area.  If someone modifies a suite to need an
additional binary without updating the test harness to furnish that binary,
the buildfarm will catch the mistake.

MSVC build system semantics should mimic the GNU make build system, not
innovate.  In released versions, vcregress.pl departs from that by using the
build tree psql instead of an installed psql.  In 9.5, it will use psql from a
temporary installation.  Where's the improvement in that?  It replaced one
inconsistency with another.

> Also, if we would
> want to have a real installcheck mode, what we should use is not the
> path to what has been built but the path to the installation that the
> Postgres instance needed is using. Now if you want to fix it if you
> fix that's incorrect I won't complain about that :)

I don't wish to.  I was content with released-branch vcregress.pl semantics.

On Mon, Jul 6, 2015 at 10:04 PM, Noah Misch wrote:
>> Also, if we would
>> want to have a real installcheck mode, what we should use is not the
>> path to what has been built but the path to the installation that the
>> Postgres instance needed is using. Now if you want to fix it if you
>> fix that's incorrect I won't complain about that :)
>
> I don't wish to.  I was content with released-branch vcregress.pl semantics.

Attached is a patch to revert to the pre-9.4 way of doing... If MSVC
should imitate what make does, perhaps you have an opinion about the
ideas I sent upthead to add a check-mode to contribcheck and
modulescheck? That's 9.6 work.
--
Michael

On Mon, Jul 06, 2015 at 11:53:58PM -0700, Michael Paquier wrote:
> On Mon, Jul 6, 2015 at 10:04 PM, Noah Misch wrote:
> >> Also, if we would
> >> want to have a real installcheck mode, what we should use is not the
> >> path to what has been built but the path to the installation that the
> >> Postgres instance needed is using. Now if you want to fix it if you
> >> fix that's incorrect I won't complain about that :)
> >
> > I don't wish to.  I was content with released-branch vcregress.pl semantics.
>
> Attached is a patch to revert to the pre-9.4 way of doing...

This is a start.  Study "git diff REL9_4_STABLE src/tools/msvc/vcregress.pl"
after applying it.  To give just one example of material left to revert, it
leaves two absolute-path "chdir" calls in a row.

> If MSVC
> should imitate what make does, perhaps you have an opinion about the
> ideas I sent upthead to add a check-mode to contribcheck and
> modulescheck? That's 9.6 work.

As you note, we lack a predefined notion of the installation path like we have
in the GNU make build.  That challenges our ability to make the two build
systems more similar than they are in 9.4.  While I have occasionally wished
for a vcregress.pl target corresponding to "make -C contrib check", it's also
one more thing to bitrot in an area that already experiences too much bitrot.
Overall, I recommend against this project.  The benefits of a correct patch
are slight, yet writing one is difficult.

On Sun, Jul 12, 2015 at 05:50:42AM -0700, Michael Paquier wrote:
> On Sun, Jul 12, 2015 at 3:57 PM, Noah Misch <noah@leadboat.com> wrote:
> > On Mon, Jul 06, 2015 at 11:53:58PM -0700, Michael Paquier wrote:
> > > Attached is a patch to revert to the pre-9.4 way of doing...
> >
> > This is a start.  Study "git diff REL9_4_STABLE src/tools/msvc/
> > vcregress.pl"
> > after applying it.  To give just one example of material left to revert, it
> > leaves two absolute-path "chdir" calls in a row.
> >
>
> Then the attached I guess addresses your concerns..

It did not; that version moved the project from 5% done to 15% done, perhaps.
I redeveloped it for several more hours last weekend and today, and I plan to
commit the attached stack of patches.

nm