testclient.exe installed under MSVC
My annual audit for executables missing Windows icons turned up these:
pginstall/bin/testclient.exe
pginstall/bin/uri-regress.exe
I was going to add the icons, but I felt the testclient.exe name is too
generic-sounding to be installed. testclient originated in commit ebc8b7d. I
recommend ceasing to install both programs under MSVC. (The GNU make build
system does not install them.) If that's unwanted for some reason, could you
rename testclient to something like libpq_test?
Thanks,
nm
On Sun, May 01, 2022 at 01:07:06AM -0700, Noah Misch wrote:
My annual audit for executables missing Windows icons turned up these:
pginstall/bin/testclient.exe
pginstall/bin/uri-regress.exeI was going to add the icons, but I felt the testclient.exe name is too
generic-sounding to be installed. testclient originated in commit ebc8b7d. I
recommend ceasing to install both programs under MSVC. (The GNU make build
system does not install them.)
But MSVC works differently. vcregress.pl does a TempInstall(), which
is a simple Install(), so isn't it going to be an issue for the tests
if these two tools are not installed anymore?
If that's unwanted for some reason, could you
rename testclient to something like libpq_test?
Yes, the renaming makes sense. I'd say to do more, and also rename
uri-regress, removing the hyphen from the binary name and prefix both
binaries with a "pg_".
--
Michael
On Sun, May 01, 2022 at 10:23:18PM +0900, Michael Paquier wrote:
On Sun, May 01, 2022 at 01:07:06AM -0700, Noah Misch wrote:
My annual audit for executables missing Windows icons turned up these:
pginstall/bin/testclient.exe
pginstall/bin/uri-regress.exeI was going to add the icons, but I felt the testclient.exe name is too
generic-sounding to be installed. testclient originated in commit ebc8b7d. I
recommend ceasing to install both programs under MSVC. (The GNU make build
system does not install them.)
See also:
a17fd67d2f2861ae0ce00d1aeefdf2facc47cd5e Build libpq test programs under MSVC.
/messages/by-id/74952229-b3b0-fe47-f958-4088529a3f21@dunslane.net MSVC build system installs extra executables
/messages/by-id/e4233934-98a6-6f76-46a0-992c0f4f1208@dunslane.net Re: set TESTDIR from perl rather than Makefile
I'm not really sure what the plan is for the TESTDIR patches. Is "vcregress
alltaptests" still an interesting patch to pursue, or is that going to be
obsoleted by meson build ?
But MSVC works differently. vcregress.pl does a TempInstall(), which
is a simple Install(), so isn't it going to be an issue for the tests
if these two tools are not installed anymore?
Andrew didn't propose any mechanism for avoiding installation of the
executables, so it would break the tests. However, at least cfbot currently
doesn't run them anyway.
One idea is if "vcregress install" accepted an option like
"vcregress install check", which would mean "install extra binaries needed for
running tests". Something maybe not much more elegant than this.
next
if ($insttype eq "client" && !grep { $_ eq $pf }
@client_program_files);
+ next if ($pf =~ /testclient|uri-regress/);
On 1 May 2022, at 15:23, Michael Paquier <michael@paquier.xyz> wrote:
On Sun, May 01, 2022 at 01:07:06AM -0700, Noah Misch wrote:
My annual audit for executables missing Windows icons turned up these:
pginstall/bin/testclient.exe
pginstall/bin/uri-regress.exeI was going to add the icons, but I felt the testclient.exe name is too
generic-sounding to be installed. testclient originated in commit ebc8b7d. I
recommend ceasing to install both programs under MSVC. (The GNU make build
system does not install them.)But MSVC works differently. vcregress.pl does a TempInstall(), which
is a simple Install(), so isn't it going to be an issue for the tests
if these two tools are not installed anymore?If that's unwanted for some reason, could you
rename testclient to something like libpq_test?Yes, the renaming makes sense. I'd say to do more, and also rename
uri-regress, removing the hyphen from the binary name and prefix both
binaries with a "pg_".
Renaming is probably the best option given how MSVC works. Using a pg_ prefix
makes them sound like actual useful tools though with (albeit small) risk for
confusion? Noah's suggestion of libpq_ is perhaps better: libpq_testclient.
--
Daniel Gustafsson https://vmware.com/
On Mon, 2022-05-02 at 15:14 +0200, Daniel Gustafsson wrote:
Using a pg_ prefix
makes them sound like actual useful tools though with (albeit small) risk for
confusion? Noah's suggestion of libpq_ is perhaps better: libpq_testclient.
+1
I also like Justin's idea of only installing the test executables when
asked to explicitly, but I don't know enough about our existing MSVC
conventions to have a strong opinion there.
Thanks,
--Jacob
On Mon, May 02, 2022 at 03:14:50PM +0200, Daniel Gustafsson wrote:
On 1 May 2022, at 15:23, Michael Paquier <michael@paquier.xyz> wrote:
On Sun, May 01, 2022 at 01:07:06AM -0700, Noah Misch wrote:My annual audit for executables missing Windows icons turned up these:
pginstall/bin/testclient.exe
pginstall/bin/uri-regress.exeI was going to add the icons, but I felt the testclient.exe name is too
generic-sounding to be installed. testclient originated in commit ebc8b7d. I
recommend ceasing to install both programs under MSVC. (The GNU make build
system does not install them.)But MSVC works differently. vcregress.pl does a TempInstall(), which
is a simple Install(), so isn't it going to be an issue for the tests
if these two tools are not installed anymore?
Resolving that would be part of any project to stop installing them.
If that's unwanted for some reason, could you
rename testclient to something like libpq_test?Yes, the renaming makes sense. I'd say to do more, and also rename
uri-regress, removing the hyphen from the binary name and prefix both
binaries with a "pg_".Renaming is probably the best option given how MSVC works. Using a pg_ prefix
makes them sound like actual useful tools though with (albeit small) risk for
confusion? Noah's suggestion of libpq_ is perhaps better: libpq_testclient.
Agreed. libpq_testclient and libpq_uri_regress sound fine.
On 3 May 2022, at 04:02, Noah Misch <noah@leadboat.com> wrote:
On Mon, May 02, 2022 at 03:14:50PM +0200, Daniel Gustafsson wrote:
Renaming is probably the best option given how MSVC works. Using a pg_ prefix
makes them sound like actual useful tools though with (albeit small) risk for
confusion? Noah's suggestion of libpq_ is perhaps better: libpq_testclient.Agreed. libpq_testclient and libpq_uri_regress sound fine.
The attached works in both Linux and (Cirrus CI) MSVC for me.
--
Daniel Gustafsson https://vmware.com/
Attachments:
0001-Rename-libpq-test-programs-with-libpq_-prefix.patchapplication/octet-stream; name=0001-Rename-libpq-test-programs-with-libpq_-prefix.patch; x-unix-mode=0644Download
From 2dfeb34944e4b167d36df3125ab8cfbe4e3550e3 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Tue, 3 May 2022 14:09:02 +0200
Subject: [PATCH] Rename libpq test programs with libpq_ prefix
The testclient and uri-regress programs in the libpq test suite had
quite generic names which didn't convey much meaning. Since they are
installed as part of the MSVC test runs, ensure that their purpose
is a little bit clearer by renaming with a libpq_ prefix.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20220501080706.GA1542365@rfd.leadboat.com
---
src/interfaces/libpq/t/001_uri.pl | 30 +++++++++----------
src/interfaces/libpq/t/002_api.pl | 2 +-
src/interfaces/libpq/test/.gitignore | 4 +--
src/interfaces/libpq/test/Makefile | 2 +-
.../test/{testclient.c => libpq_testclient.c} | 4 +--
.../{uri-regress.c => libpq_uri-regress.c} | 8 ++---
src/tools/msvc/Mkvcbuild.pm | 8 ++---
7 files changed, 29 insertions(+), 29 deletions(-)
rename src/interfaces/libpq/test/{testclient.c => libpq_testclient.c} (88%)
rename src/interfaces/libpq/test/{uri-regress.c => libpq_uri-regress.c} (90%)
diff --git a/src/interfaces/libpq/t/001_uri.pl b/src/interfaces/libpq/t/001_uri.pl
index 1d595c0529..a50ae001cb 100644
--- a/src/interfaces/libpq/t/001_uri.pl
+++ b/src/interfaces/libpq/t/001_uri.pl
@@ -72,7 +72,7 @@ my @tests = (
],
[
q{postgresql://host/db?u%7aer=someotheruser&port=12345}, q{},
- q{uri-regress: invalid URI query parameter: "uzer"},
+ q{libpq_uri-regress: invalid URI query parameter: "uzer"},
],
[
q{postgresql://host:12345?user=uri-user},
@@ -114,63 +114,63 @@ my @tests = (
],
[
q{postgresql://host?uzer=}, q{},
- q{uri-regress: invalid URI query parameter: "uzer"},
+ q{libpq_uri-regress: invalid URI query parameter: "uzer"},
],
[
q{postgre://},
q{},
- q{uri-regress: missing "=" after "postgre://" in connection info string},
+ q{libpq_uri-regress: missing "=" after "postgre://" in connection info string},
],
[
q{postgres://[::1},
q{},
- q{uri-regress: end of string reached when looking for matching "]" in IPv6 host address in URI: "postgres://[::1"},
+ q{libpq_uri-regress: end of string reached when looking for matching "]" in IPv6 host address in URI: "postgres://[::1"},
],
[
q{postgres://[]},
q{},
- q{uri-regress: IPv6 host address may not be empty in URI: "postgres://[]"},
+ q{libpq_uri-regress: IPv6 host address may not be empty in URI: "postgres://[]"},
],
[
q{postgres://[::1]z},
q{},
- q{uri-regress: unexpected character "z" at position 17 in URI (expected ":" or "/"): "postgres://[::1]z"},
+ q{libpq_uri-regress: unexpected character "z" at position 17 in URI (expected ":" or "/"): "postgres://[::1]z"},
],
[
q{postgresql://host?zzz},
q{},
- q{uri-regress: missing key/value separator "=" in URI query parameter: "zzz"},
+ q{libpq_uri-regress: missing key/value separator "=" in URI query parameter: "zzz"},
],
[
q{postgresql://host?value1&value2},
q{},
- q{uri-regress: missing key/value separator "=" in URI query parameter: "value1"},
+ q{libpq_uri-regress: missing key/value separator "=" in URI query parameter: "value1"},
],
[
q{postgresql://host?key=key=value},
q{},
- q{uri-regress: extra key/value separator "=" in URI query parameter: "key"},
+ q{libpq_uri-regress: extra key/value separator "=" in URI query parameter: "key"},
],
[
q{postgres://host?dbname=%XXfoo}, q{},
- q{uri-regress: invalid percent-encoded token: "%XXfoo"},
+ q{libpq_uri-regress: invalid percent-encoded token: "%XXfoo"},
],
[
q{postgresql://a%00b},
q{},
- q{uri-regress: forbidden value %00 in percent-encoded value: "a%00b"},
+ q{libpq_uri-regress: forbidden value %00 in percent-encoded value: "a%00b"},
],
[
q{postgresql://%zz}, q{},
- q{uri-regress: invalid percent-encoded token: "%zz"},
+ q{libpq_uri-regress: invalid percent-encoded token: "%zz"},
],
[
q{postgresql://%1}, q{},
- q{uri-regress: invalid percent-encoded token: "%1"},
+ q{libpq_uri-regress: invalid percent-encoded token: "%1"},
],
[
q{postgresql://%}, q{},
- q{uri-regress: invalid percent-encoded token: "%"},
+ q{libpq_uri-regress: invalid percent-encoded token: "%"},
],
[ q{postgres://@host}, q{host='host' (inet)}, q{}, ],
[ q{postgres://host:/}, q{host='host' (inet)}, q{}, ],
@@ -224,7 +224,7 @@ sub test_uri
$expect{'exit'} = $expect{stderr} eq '';
- my $cmd = [ 'uri-regress', $uri ];
+ my $cmd = [ 'libpq_uri-regress', $uri ];
$result{exit} = IPC::Run::run $cmd, '>', \$result{stdout}, '2>',
\$result{stderr};
diff --git a/src/interfaces/libpq/t/002_api.pl b/src/interfaces/libpq/t/002_api.pl
index 7c6c5788a0..8b3355e6dd 100644
--- a/src/interfaces/libpq/t/002_api.pl
+++ b/src/interfaces/libpq/t/002_api.pl
@@ -6,7 +6,7 @@ use PostgreSQL::Test::Utils;
use Test::More;
# Test PQsslAttribute(NULL, "library")
-my ($out, $err) = run_command(['testclient', '--ssl']);
+my ($out, $err) = run_command(['libpq_testclient', '--ssl']);
if ($ENV{with_ssl} eq 'openssl')
{
diff --git a/src/interfaces/libpq/test/.gitignore b/src/interfaces/libpq/test/.gitignore
index 4b17210483..bbc7372e29 100644
--- a/src/interfaces/libpq/test/.gitignore
+++ b/src/interfaces/libpq/test/.gitignore
@@ -1,2 +1,2 @@
-/testclient
-/uri-regress
+/libpq_testclient
+/libpq_uri-regress
diff --git a/src/interfaces/libpq/test/Makefile b/src/interfaces/libpq/test/Makefile
index 1d45be0c37..a88db0e86a 100644
--- a/src/interfaces/libpq/test/Makefile
+++ b/src/interfaces/libpq/test/Makefile
@@ -11,7 +11,7 @@ endif
override CPPFLAGS := -I$(libpq_srcdir) $(CPPFLAGS)
LDFLAGS_INTERNAL += $(libpq_pgport)
-PROGS = testclient uri-regress
+PROGS = libpq_testclient libpq_uri-regress
all: $(PROGS)
diff --git a/src/interfaces/libpq/test/testclient.c b/src/interfaces/libpq/test/libpq_testclient.c
similarity index 88%
rename from src/interfaces/libpq/test/testclient.c
rename to src/interfaces/libpq/test/libpq_testclient.c
index 2c730d83fa..d945bacf1b 100644
--- a/src/interfaces/libpq/test/testclient.c
+++ b/src/interfaces/libpq/test/libpq_testclient.c
@@ -1,11 +1,11 @@
/*
- * testclient.c
+ * libpq_testclient.c
* A test program for the libpq public API
*
* Copyright (c) 2022, PostgreSQL Global Development Group
*
* IDENTIFICATION
- * src/interfaces/libpq/test/testclient.c
+ * src/interfaces/libpq/test/libpq_testclient.c
*/
#include "postgres_fe.h"
diff --git a/src/interfaces/libpq/test/uri-regress.c b/src/interfaces/libpq/test/libpq_uri-regress.c
similarity index 90%
rename from src/interfaces/libpq/test/uri-regress.c
rename to src/interfaces/libpq/test/libpq_uri-regress.c
index c5ef33c46b..3c0561ab12 100644
--- a/src/interfaces/libpq/test/uri-regress.c
+++ b/src/interfaces/libpq/test/libpq_uri-regress.c
@@ -1,5 +1,5 @@
/*
- * uri-regress.c
+ * libpq_uri-regress.c
* A test program for libpq URI format
*
* This is a helper for libpq conninfo regression testing. It takes a single
@@ -10,7 +10,7 @@
* Portions Copyright (c) 2012-2022, PostgreSQL Global Development Group
*
* IDENTIFICATION
- * src/interfaces/libpq/test/uri-regress.c
+ * src/interfaces/libpq/test/libpq_uri-regress.c
*/
#include "postgres_fe.h"
@@ -33,14 +33,14 @@ main(int argc, char *argv[])
opts = PQconninfoParse(argv[1], &errmsg);
if (opts == NULL)
{
- fprintf(stderr, "uri-regress: %s", errmsg);
+ fprintf(stderr, "libpq_uri-regress: %s", errmsg);
return 1;
}
defs = PQconndefaults();
if (defs == NULL)
{
- fprintf(stderr, "uri-regress: cannot fetch default options\n");
+ fprintf(stderr, "libpq_uri-regress: cannot fetch default options\n");
return 1;
}
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index fc04e1db8e..76b7ee6b95 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -285,17 +285,17 @@ sub mkvcbuild
$libpqwalreceiver->AddReference($postgres, $libpq);
my $libpq_testclient =
- $solution->AddProject('testclient', 'exe', 'misc',
+ $solution->AddProject('libpq_testclient', 'exe', 'misc',
'src/interfaces/libpq/test');
- $libpq_testclient->AddFile('src/interfaces/libpq/test/testclient.c');
+ $libpq_testclient->AddFile('src/interfaces/libpq/test/libpq_testclient.c');
$libpq_testclient->AddIncludeDir('src/interfaces/libpq');
$libpq_testclient->AddReference($libpgport, $libpq);
$libpq_testclient->AddLibrary('ws2_32.lib');
my $libpq_uri_regress =
- $solution->AddProject('uri-regress', 'exe', 'misc',
+ $solution->AddProject('libpq_uri-regress', 'exe', 'misc',
'src/interfaces/libpq/test');
- $libpq_uri_regress->AddFile('src/interfaces/libpq/test/uri-regress.c');
+ $libpq_uri_regress->AddFile('src/interfaces/libpq/test/libpq_uri-regress.c');
$libpq_uri_regress->AddIncludeDir('src/interfaces/libpq');
$libpq_uri_regress->AddReference($libpgport, $libpq);
$libpq_uri_regress->AddLibrary('ws2_32.lib');
--
2.32.0 (Apple Git-132)
On Tue, May 03, 2022 at 03:04:26PM +0200, Daniel Gustafsson wrote:
On 3 May 2022, at 04:02, Noah Misch <noah@leadboat.com> wrote:
On Mon, May 02, 2022 at 03:14:50PM +0200, Daniel Gustafsson wrote:Renaming is probably the best option given how MSVC works. Using a pg_ prefix
makes them sound like actual useful tools though with (albeit small) risk for
confusion? Noah's suggestion of libpq_ is perhaps better: libpq_testclient.Agreed. libpq_testclient and libpq_uri_regress sound fine.
The attached works in both Linux and (Cirrus CI) MSVC for me.
Michael Paquier recommended s/-/_/ for uri-regress, and I agree with that.
What do you think?
On 2022-May-03, Noah Misch wrote:
Michael Paquier recommended s/-/_/ for uri-regress, and I agree with that.
What do you think?
libpq_uri-regress is horrible, so +1 for that. I would personally
rename more thoroughly (say pq_uri_test), but I doubt it's worth the
bikeshedding effort.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"At least to kernel hackers, who really are human, despite occasional
rumors to the contrary" (LWN.net)
On 3 May 2022, at 15:58, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2022-May-03, Noah Misch wrote:
Michael Paquier recommended s/-/_/ for uri-regress, and I agree with that.
What do you think?libpq_uri-regress is horrible, so +1 for that.
Agreed, I'll do that before pushing.
--
Daniel Gustafsson https://vmware.com/
On 2022-05-01 Su 09:23, Michael Paquier wrote:
On Sun, May 01, 2022 at 01:07:06AM -0700, Noah Misch wrote:
My annual audit for executables missing Windows icons turned up these:
pginstall/bin/testclient.exe
pginstall/bin/uri-regress.exeI was going to add the icons, but I felt the testclient.exe name is too
generic-sounding to be installed. testclient originated in commit ebc8b7d. I
recommend ceasing to install both programs under MSVC. (The GNU make build
system does not install them.)But MSVC works differently. vcregress.pl does a TempInstall(), which
is a simple Install(), so isn't it going to be an issue for the tests
if these two tools are not installed anymore?If that's unwanted for some reason, could you
rename testclient to something like libpq_test?Yes, the renaming makes sense. I'd say to do more, and also rename
uri-regress, removing the hyphen from the binary name and prefix both
binaries with a "pg_".
I've complained before about binaries that are installed under MSVC
where the equivalent are not installed under Unix or msys{2}.
I think we should make the standard MSVC install look as much like the
standard Unix/msys install as possible. If we need a test mode that
installs a few extra things then that can be managed fairly simply I
think. I'm prepared to help out with that.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On 3 May 2022, at 16:50, Daniel Gustafsson <daniel@yesql.se> wrote:
On 3 May 2022, at 15:58, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2022-May-03, Noah Misch wrote:
Michael Paquier recommended s/-/_/ for uri-regress, and I agree with that.
What do you think?libpq_uri-regress is horrible, so +1 for that.
Agreed, I'll do that before pushing.
Done that way.
--
Daniel Gustafsson https://vmware.com/
On 4 May 2022, at 02:34, Andrew Dunstan <andrew@dunslane.net> wrote:
I think we should make the standard MSVC install look as much like the
standard Unix/msys install as possible.
+1
--
Daniel Gustafsson https://vmware.com/