Reduce the number of special cases to build contrib modules on windows

Hi,

On 2020-11-02 20:34:28 +1300, David Rowley wrote:

It might be nice if we could reduce some of those special cases so
that:

a) We reduce the amount of work specific to windows when we add new
contrib modules, and;
b) We can work towards a better way for people to build their own
extensions on windows.

A worthy goal.

diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 90594bd41b..491a465e2f 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -32,16 +32,13 @@ my $libpq;
my @unlink_on_exit;

# Set of variables for modules in contrib/ and src/test/modules/
-my $contrib_defines = { 'refint' => 'REFINT_VERBOSE' };
-my @contrib_uselibpq = ('dblink', 'oid2name', 'postgres_fdw', 'vacuumlo');
-my @contrib_uselibpgport   = ('oid2name', 'pg_standby', 'vacuumlo');
-my @contrib_uselibpgcommon = ('oid2name', 'pg_standby', 'vacuumlo');
+my $contrib_defines = undef;
+my @contrib_uselibpq = undef;
+my @contrib_uselibpgport   = ('pg_standby');
+my @contrib_uselibpgcommon = ('pg_standby');
my $contrib_extralibs      = undef;
my $contrib_extraincludes = { 'dblink' => ['src/backend'] };
-my $contrib_extrasource = {
-	'cube' => [ 'contrib/cube/cubescan.l', 'contrib/cube/cubeparse.y' ],
-	'seg'  => [ 'contrib/seg/segscan.l',   'contrib/seg/segparse.y' ],
-};
+my $contrib_extrasource = undef;

Hm - Is that all the special case stuff we get rid of?

What's with the now unef'd arrays/hashes? First, wouldn't an empty array be
more appropriate? Second, can we just get rid of them?

And why is the special stuff for pg_standby still needed?

my @contrib_excludes = (
'bool_plperl',      'commit_ts',
'hstore_plperl',    'hstore_plpython',
@@ -163,7 +160,7 @@ sub mkvcbuild
$postgres = $solution->AddProject('postgres', 'exe', '', 'src/backend');
$postgres->AddIncludeDir('src/backend');
$postgres->AddDir('src/backend/port/win32');
-	$postgres->AddFile('src/backend/utils/fmgrtab.c');
+	$postgres->AddFile('src/backend/utils/fmgrtab.c', 1);
$postgres->ReplaceFile('src/backend/port/pg_sema.c',
'src/backend/port/win32_sema.c');
$postgres->ReplaceFile('src/backend/port/pg_shmem.c',
@@ -316,8 +313,8 @@ sub mkvcbuild

Why do so many places need this new parameter? Looks like all explicit
calls use it? Can't we just use it by default, using a separate function
for the internal cases? Would make this a lot more readable...

my $isolation_tester =
$solution->AddProject('isolationtester', 'exe', 'misc');
-	$isolation_tester->AddFile('src/test/isolation/isolationtester.c');
-	$isolation_tester->AddFile('src/test/isolation/specparse.y');
-	$isolation_tester->AddFile('src/test/isolation/specscanner.l');
-	$isolation_tester->AddFile('src/test/isolation/specparse.c');
+	$isolation_tester->AddFile('src/test/isolation/isolationtester.c', 1);
+	$isolation_tester->AddFile('src/test/isolation/specparse.y', 1);
+	$isolation_tester->AddFile('src/test/isolation/specscanner.l', 1);
+	$isolation_tester->AddFile('src/test/isolation/specparse.c', 1);
$isolation_tester->AddIncludeDir('src/test/isolation');
$isolation_tester->AddIncludeDir('src/port');
$isolation_tester->AddIncludeDir('src/test/regress');
@@ -342,8 +339,8 @@ sub mkvcbuild

Why aren't these dealth with using the .c->.l/.y logic you added?

+	# Process custom compiler flags
+	if ($mf =~ /^PG_CPPFLAGS\s*=\s*(.*)$/mg)

Probably worth mentioning in pgxs.mk or such.

+	{
+		foreach my $flag (split /\s+/, $1)
+		{
+			if ($flag =~ /^-D(.*)$/)
+			{
+				foreach my $proj (@projects)
+				{
+					$proj->AddDefine($1);
+				}
+			}
+			elsif ($flag =~ /^-I(.*)$/)
+			{
+				foreach my $proj (@projects)
+				{
+					if ($1 eq '$(libpq_srcdir)')
+					{
+						$proj->AddIncludeDir('src\interfaces\libpq');
+						$proj->AddReference($libpq);
+					}

Why just libpq?

+# Handle makefile rules for when file to be added to the project
+# does not exist.  Returns 1 when the original file add should be
+# skipped.
+sub AdditionalFileRules
+{
+	my $self = shift;
+	my $fname = shift;
+	my ($ext) = $fname =~ /(\.[^.]+)$/;
+
+	# For missing .c files, check if a .l file of the same name
+	# exists and add that too.
+	if ($ext eq ".c")
+	{
+		my $filenoext = $fname;
+		$filenoext =~ s{\.[^.]+$}{};
+		if (-e "$filenoext.l")
+		{
+			AddFile($self, "$filenoext.l", 0);
+			return 1;
+		}
+		if (-e "$filenoext.y")
+		{
+			AddFile($self, "$filenoext.y", 0);
+			return 0;
+		}
+	}

Aren't there related rules for .h?

Greetings,

Andres Freund

Thank you for looking at this.

On Tue, 3 Nov 2020 at 09:49, Andres Freund <andres@anarazel.de> wrote:

diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 90594bd41b..491a465e2f 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -32,16 +32,13 @@ my $libpq;
my @unlink_on_exit;

# Set of variables for modules in contrib/ and src/test/modules/
-my $contrib_defines = { 'refint' => 'REFINT_VERBOSE' };
-my @contrib_uselibpq = ('dblink', 'oid2name', 'postgres_fdw', 'vacuumlo');
-my @contrib_uselibpgport   = ('oid2name', 'pg_standby', 'vacuumlo');
-my @contrib_uselibpgcommon = ('oid2name', 'pg_standby', 'vacuumlo');
+my $contrib_defines = undef;
+my @contrib_uselibpq = undef;
+my @contrib_uselibpgport   = ('pg_standby');
+my @contrib_uselibpgcommon = ('pg_standby');
my $contrib_extralibs      = undef;
my $contrib_extraincludes = { 'dblink' => ['src/backend'] };
-my $contrib_extrasource = {
-     'cube' => [ 'contrib/cube/cubescan.l', 'contrib/cube/cubeparse.y' ],
-     'seg'  => [ 'contrib/seg/segscan.l',   'contrib/seg/segparse.y' ],
-};
+my $contrib_extrasource = undef;

Hm - Is that all the special case stuff we get rid of?

What else did you have in mind?

What's with the now unef'd arrays/hashes? First, wouldn't an empty array be
more appropriate? Second, can we just get rid of them?

Yes, those should be empty hashtables/arrays. I've changed that.

We could get rid of the variables too. I've just left them in there
for now as I wasn't sure if it might be a good idea to keep them for
if we really need to brute force something in the future. I found
parsing makefiles quite tedious, so it didn't seem unrealistic to me
that someone might struggle in the future to make something work.

And why is the special stuff for pg_standby still needed?

I'm not much of an expert, but I didn't see anything in the makefile
for pg_standby that indicates we should link libpgport or libpgcommon.
It would be good if someone could explain how that works.

my @contrib_excludes = (
'bool_plperl',      'commit_ts',
'hstore_plperl',    'hstore_plpython',
@@ -163,7 +160,7 @@ sub mkvcbuild
$postgres = $solution->AddProject('postgres', 'exe', '', 'src/backend');
$postgres->AddIncludeDir('src/backend');
$postgres->AddDir('src/backend/port/win32');
-     $postgres->AddFile('src/backend/utils/fmgrtab.c');
+     $postgres->AddFile('src/backend/utils/fmgrtab.c', 1);
$postgres->ReplaceFile('src/backend/port/pg_sema.c',
'src/backend/port/win32_sema.c');
$postgres->ReplaceFile('src/backend/port/pg_shmem.c',
@@ -316,8 +313,8 @@ sub mkvcbuild

Why do so many places need this new parameter? Looks like all explicit
calls use it? Can't we just use it by default, using a separate function
for the internal cases? Would make this a lot more readable...

That makes sense. I've updated the patch to have AddFile() add any
additional files always then I've also added a new function named
AddFileConditional which does what AddFile(..., 0) did.

my $isolation_tester =
$solution->AddProject('isolationtester', 'exe', 'misc');
-     $isolation_tester->AddFile('src/test/isolation/isolationtester.c');
-     $isolation_tester->AddFile('src/test/isolation/specparse.y');
-     $isolation_tester->AddFile('src/test/isolation/specscanner.l');
-     $isolation_tester->AddFile('src/test/isolation/specparse.c');
+     $isolation_tester->AddFile('src/test/isolation/isolationtester.c', 1);
+     $isolation_tester->AddFile('src/test/isolation/specparse.y', 1);
+     $isolation_tester->AddFile('src/test/isolation/specscanner.l', 1);
+     $isolation_tester->AddFile('src/test/isolation/specparse.c', 1);
$isolation_tester->AddIncludeDir('src/test/isolation');
$isolation_tester->AddIncludeDir('src/port');
$isolation_tester->AddIncludeDir('src/test/regress');
@@ -342,8 +339,8 @@ sub mkvcbuild

Why aren't these dealth with using the .c->.l/.y logic you added?

Yeah, some of those could be removed. I mostly only paid attention to
contrib though.

+     # Process custom compiler flags
+     if ($mf =~ /^PG_CPPFLAGS\s*=\s*(.*)$/mg)

Probably worth mentioning in pgxs.mk or such.

I'm not quite sure I understand what you mean here.

+     {
+             foreach my $flag (split /\s+/, $1)
+             {
+                     if ($flag =~ /^-D(.*)$/)
+                     {
+                             foreach my $proj (@projects)
+                             {
+                                     $proj->AddDefine($1);
+                             }
+                     }
+                     elsif ($flag =~ /^-I(.*)$/)
+                     {
+                             foreach my $proj (@projects)
+                             {
+                                     if ($1 eq '$(libpq_srcdir)')
+                                     {
+                                             $proj->AddIncludeDir('src\interfaces\libpq');
+                                             $proj->AddReference($libpq);
+                                     }

Why just libpq?

I've only gone as far as making the existing contrib modules build.
Likely there's more to be done there.

+# Handle makefile rules for when file to be added to the project
+# does not exist.  Returns 1 when the original file add should be
+# skipped.
+sub AdditionalFileRules
+{
+     my $self = shift;
+     my $fname = shift;
+     my ($ext) = $fname =~ /(\.[^.]+)$/;
+
+     # For missing .c files, check if a .l file of the same name
+     # exists and add that too.
+     if ($ext eq ".c")
+     {
+             my $filenoext = $fname;
+             $filenoext =~ s{\.[^.]+$}{};
+             if (-e "$filenoext.l")
+             {
+                     AddFile($self, "$filenoext.l", 0);
+                     return 1;
+             }
+             if (-e "$filenoext.y")
+             {
+                     AddFile($self, "$filenoext.y", 0);
+                     return 0;
+             }
+     }

Aren't there related rules for .h?

I've only gone as far as making the existing contrib modules build.
Likely there's more to be done there.

David

On 2020-Nov-06, David Rowley wrote:

+# Handle makefile rules for when file to be added to the project
+# does not exist.  Returns 1 when the original file add should be
+# skipped.
+sub FindAndAddAdditionalFiles
+{
+	my $self = shift;
+	my $fname = shift;
+	my ($ext) = $fname =~ /(\.[^.]+)$/;
+
+	# For .c files, check if a .l file of the same name exists and add that
+	# too.
+	if ($ext eq ".c")
+	{
+		my $filenoext = $fname;
+		$filenoext =~ s{\.[^.]+$}{};

I think you can make this simpler by capturing both the basename and the
extension in one go. For example,

$fname =~ /(.*)(\.[^.]+)$/;
$filenoext = $1;
$ext = $2;

so you avoid the second =~ statement.

+		if (-e "$filenoext.l")
+		{
+			AddFileConditional($self, "$filenoext.l");
+			return 1;
+		}
+		if (-e "$filenoext.y")
+		{
+			AddFileConditional($self, "$filenoext.y");

Maybe DRY like

for my $ext (".l", ".y") {
my $file = $filenoext . $ext;
AddFileConditional($self, $file) if -f $file;
return 1;
}

Note: comment says "check if a .l file" and then checks both .l and .y.
Probably want to update the comment ...

On Tue, 10 Nov 2020 at 03:07, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2020-Nov-06, David Rowley wrote:

+# Handle makefile rules for when file to be added to the project
+# does not exist.  Returns 1 when the original file add should be
+# skipped.
+sub FindAndAddAdditionalFiles
+{
+     my $self = shift;
+     my $fname = shift;
+     my ($ext) = $fname =~ /(\.[^.]+)$/;
+
+     # For .c files, check if a .l file of the same name exists and add that
+     # too.
+     if ($ext eq ".c")
+     {
+             my $filenoext = $fname;
+             $filenoext =~ s{\.[^.]+$}{};

I think you can make this simpler by capturing both the basename and the
extension in one go. For example,

$fname =~ /(.*)(\.[^.]+)$/;
$filenoext = $1;
$ext = $2;

so you avoid the second =~ statement.

Thanks. That's neater.

+             if (-e "$filenoext.l")
+             {
+                     AddFileConditional($self, "$filenoext.l");
+                     return 1;
+             }
+             if (-e "$filenoext.y")
+             {
+                     AddFileConditional($self, "$filenoext.y");

Maybe DRY like

for my $ext (".l", ".y") {
my $file = $filenoext . $ext;
AddFileConditional($self, $file) if -f $file;
return 1;
}

I did adapt that part of the code, but not exactly to what's above.
The return there would cause us to return from the function after the
first iteration.

Note: comment says "check if a .l file" and then checks both .l and .y.
Probably want to update the comment ...

Updated.

I've attached the v3 patch.

I'm still working through some small differences in some of the
.vcxproj files. I've been comparing these by copying *.vcxproj out to
another directory with patched and unpatched then diffing the
directory. See attached txt file with those diffs. Here's a summary of
some of them:

1. There are a few places that libpq gets linked where it previously did not.
2. REFINT_VERBOSE gets defined in a few more places than it did
previously. This makes it closer to what happens on Linux anyway, if
you look at the Make output from contrib/spi/Makefile you'll see
-DREFINT_VERBOSE in there for autoinc
3. LOWER_NODE gets defined in ltree now where it wasn't before. It's
defined on Linux. Unsure why it wasn't before on Windows.

David

On Wed, Nov 11, 2020 at 11:01:57AM +1300, David Rowley wrote:

I'm still working through some small differences in some of the
.vcxproj files. I've been comparing these by copying *.vcxproj out to
another directory with patched and unpatched then diffing the
directory. See attached txt file with those diffs. Here's a summary of
some of them:

Thanks. It would be good to not have those diffs if not necessary.

1. There are a few places that libpq gets linked where it previously did not.

It seems to me that your patch is doing the right thing for adminpack
and that its Makefile has no need to include a reference to libpq
source path, no?

For dblink and postgres_fdw, the duplication comes from PG_CPPFLAGS.
It does not matter much in practice, but it would be nice to not have
unnecessary data in the project files. One thing that could be done
is to make Project.pm more aware of the uniqueness of the elements
included. But, do we really need -I$(libpq_srcdir) there anyway?
From what I can see, we have all the paths in -I we'd actually need
with or without USE_PGXS.

2. REFINT_VERBOSE gets defined in a few more places than it did
previously. This makes it closer to what happens on Linux anyway, if
you look at the Make output from contrib/spi/Makefile you'll see
-DREFINT_VERBOSE in there for autoinc.

Indeed.

3. LOWER_NODE gets defined in ltree now where it wasn't before. It's
defined on Linux. Unsure why it wasn't before on Windows.

Your patch is grabbing the value of PG_CPPFLAGS from ltree's
Makefile, which is fine. We may be able to remove this flag and rely
on pg_tolower() instead in the long run? I am not sure about
FLG_CANLOOKSIGN() though.
--
Michael

On Wed, 11 Nov 2020 at 13:44, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Nov 11, 2020 at 11:01:57AM +1300, David Rowley wrote:

I'm still working through some small differences in some of the
.vcxproj files. I've been comparing these by copying *.vcxproj out to
another directory with patched and unpatched then diffing the
directory. See attached txt file with those diffs. Here's a summary of
some of them:

Thanks. It would be good to not have those diffs if not necessary.

1. There are a few places that libpq gets linked where it previously did not.

It seems to me that your patch is doing the right thing for adminpack
and that its Makefile has no need to include a reference to libpq
source path, no?

Yeah. Likely a separate commit should remove the -I$(libpq_srcdir)
from adminpack and old_snapshot

For dblink and postgres_fdw, the duplication comes from PG_CPPFLAGS.
It does not matter much in practice, but it would be nice to not have
unnecessary data in the project files. One thing that could be done
is to make Project.pm more aware of the uniqueness of the elements
included. But, do we really need -I$(libpq_srcdir) there anyway?
From what I can see, we have all the paths in -I we'd actually need
with or without USE_PGXS.

I've changed the patch to do that for the includes. I'm now putting
the list of include directories in a hash table to get rid of the
duplicates. This does shuffle the order of them around a bit. I've
done the same for references too.

3. LOWER_NODE gets defined in ltree now where it wasn't before. It's
defined on Linux. Unsure why it wasn't before on Windows.

Your patch is grabbing the value of PG_CPPFLAGS from ltree's
Makefile, which is fine. We may be able to remove this flag and rely
on pg_tolower() instead in the long run? I am not sure about
FLG_CANLOOKSIGN() though.

I didn't look in detail, but it looks like if we define LOWER_NODE on
Windows that it might break pg_upgrade. I guess you could say it's
partially broken now as the behaviour there will depend on if you
build using Visual Studio or cygwin. We'd define LOWER_NODE on cygwin
but not on VS. Looks like a pg_upgrade might be problematic there
today.

It feels a bit annoying to add some special case to the script to
maintain the status quo there. An alternative to that would be to
modify the .c code at #ifdef LOWER_NODE to also check we're not
building on VS. Neither option seems nice.

I've attached the updated patch and also a diff showing the changes in
the *.vcxproj files.

There are quite a few places where the hash table code for includes
and references gets rid of duplicates that already exist today. For
example pgbench.vcxproj references libpgport.vcxproj and
libpgcommon.vcxproj twice.

David

On Tue, Dec 22, 2020 at 11:24:40PM +1300, David Rowley wrote:

On Wed, 11 Nov 2020 at 13:44, Michael Paquier <michael@paquier.xyz> wrote:

It seems to me that your patch is doing the right thing for adminpack
and that its Makefile has no need to include a reference to libpq
source path, no?

Yeah. Likely a separate commit should remove the -I$(libpq_srcdir)
from adminpack and old_snapshot

I have begun a new thread about this point as that's a separate
topic. I did not see other places in need of a similar cleanup:
/messages/by-id/X+LQpfLyk7jgzUki@paquier.xyz

I didn't look in detail, but it looks like if we define LOWER_NODE on
Windows that it might break pg_upgrade. I guess you could say it's
partially broken now as the behaviour there will depend on if you
build using Visual Studio or cygwin. We'd define LOWER_NODE on cygwin
but not on VS. Looks like a pg_upgrade might be problematic there
today.

It feels a bit annoying to add some special case to the script to
maintain the status quo there. An alternative to that would be to
modify the .c code at #ifdef LOWER_NODE to also check we're not
building on VS. Neither option seems nice.

Hmm. It seems that you are right here. This influences lquery
parsing so it may be nasty and this exists since ltree is present in
the tree (2002). I think that I would choose the update in the C code
and remove LOWER_NODE while keeping the scripts clean, and documenting
directly in the code why this compatibility issue exists.
REFINT_VERBOSE is no problem, fortunately.

I've attached the updated patch and also a diff showing the changes in
the *.vcxproj files.

Thanks!

There are quite a few places where the hash table code for includes
and references gets rid of duplicates that already exist today. For
example pgbench.vcxproj references libpgport.vcxproj and
libpgcommon.vcxproj twice.

The diffs look clean. dblink has lost src/backend/, there are the
additions of REFINT_VERBOSE and LOWER_NODE but the bulk of the diffs
comes from a change in the order of items listed, while removing
duplicates.

I have tested your patch, and this is causing compilation failures for
hstore_plpython, jsonb_plpython and ltree_plpython. So
AddTransformModule is missing something here when compiling with
Python.
--
Michael

On Wed, 23 Dec 2020 at 18:46, Michael Paquier <michael@paquier.xyz> wrote:

I have begun a new thread about this point as that's a separate
topic. I did not see other places in need of a similar cleanup:
/messages/by-id/X+LQpfLyk7jgzUki@paquier.xyz

Thanks. I'll look at that shortly.

I didn't look in detail, but it looks like if we define LOWER_NODE on
Windows that it might break pg_upgrade. I guess you could say it's
partially broken now as the behaviour there will depend on if you
build using Visual Studio or cygwin. We'd define LOWER_NODE on cygwin
but not on VS. Looks like a pg_upgrade might be problematic there
today.

It feels a bit annoying to add some special case to the script to
maintain the status quo there. An alternative to that would be to
modify the .c code at #ifdef LOWER_NODE to also check we're not
building on VS. Neither option seems nice.

Hmm. It seems that you are right here. This influences lquery
parsing so it may be nasty and this exists since ltree is present in
the tree (2002). I think that I would choose the update in the C code
and remove LOWER_NODE while keeping the scripts clean, and documenting
directly in the code why this compatibility issue exists.
REFINT_VERBOSE is no problem, fortunately.

I ended up modifying each place in the C code where we check
LOWER_NODE. I found 2 places, one in crc32.c and another in ltree.h.
I added the same comment to both to explain why there's a check for
!defined(_MSC_VER) there. I'm not particularly happy about this code,
but I don't really see what else to do right now.

I have tested your patch, and this is causing compilation failures for
hstore_plpython, jsonb_plpython and ltree_plpython. So
AddTransformModule is missing something here when compiling with
Python.

Oh thanks for finding that. That was due to some incorrect Perl code
I'd written to add the includes from one project into another. Fixed
by:

-       $p->AddIncludeDir(join(";", $pl_proj->{includes}));
+       foreach my $inc (keys %{ $pl_proj->{includes} } )
+       {
+               $p->AddIncludeDir($inc);
+       }
+

David

On Wed, 30 Dec 2020 at 10:03, David Rowley <dgrowleyml@gmail.com> wrote:

On Wed, 23 Dec 2020 at 18:46, Michael Paquier <michael@paquier.xyz> wrote:

I have tested your patch, and this is causing compilation failures for
hstore_plpython, jsonb_plpython and ltree_plpython. So
AddTransformModule is missing something here when compiling with
Python.

Oh thanks for finding that. That was due to some incorrect Perl code
I'd written to add the includes from one project into another. Fixed
by:

I accidentally attached the wrong patch before. Now attaching the correct one.

David

On Wed, Dec 30, 2020 at 10:07:29AM +1300, David Rowley wrote:

-#ifdef LOWER_NODE
+/*
+ * Below we ignore the fact that LOWER_NODE is defined when compiling with
+ * MSVC.  The reason for this is that earlier versions of the MSVC build
+ * scripts failed to define LOWER_NODE.  More recent version of the MSVC
+ * build scripts parse makefiles which results in LOWER_NODE now being
+ * defined.  We check for _MSC_VER here so as not to break pg_upgrade when
+ * upgrading from versions MSVC versions where LOWER_NODE was not defined.
+ */
+#if defined(LOWER_NODE) && !defined(_MSC_VER)
#include <ctype.h>
#define TOLOWER(x)	tolower((unsigned char) (x))
#else

While on it, do you think that it would be more readable if we remove
completely LOWER_NODE and use only a check based on _MSC_VER for those
two files in ltree? This could also be handled as a separate change.

+	foreach my $line (split /\n/, $mf)
+	{
+		if ($line =~ /^[A-Za-z0-9_]*\.o:\s(.*)/)
+		{
+			foreach my $file (split /\s+/, $1)
+			{
+				foreach my $proj (@projects)
+				{
+					$proj->AddFileConditional("$subdir/$n/$file");
+				}
+			}
+		}
+	}

Looking closer at this change, I don't think that this is completely
correct and that could become a trap. This is adding quite a bit of
complexity to take care of contrib_extrasource getting empty, and it
actually overlaps with the handling of OBJS done in AddDir(), no?
--
Michael

Thank you for having another look at this.

On Tue, 12 Jan 2021 at 20:18, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Dec 30, 2020 at 10:07:29AM +1300, David Rowley wrote:

-#ifdef LOWER_NODE
+/*
+ * Below we ignore the fact that LOWER_NODE is defined when compiling with
+ * MSVC.  The reason for this is that earlier versions of the MSVC build
+ * scripts failed to define LOWER_NODE.  More recent version of the MSVC
+ * build scripts parse makefiles which results in LOWER_NODE now being
+ * defined.  We check for _MSC_VER here so as not to break pg_upgrade when
+ * upgrading from versions MSVC versions where LOWER_NODE was not defined.
+ */
+#if defined(LOWER_NODE) && !defined(_MSC_VER)
#include <ctype.h>
#define TOLOWER(x)   tolower((unsigned char) (x))
#else

While on it, do you think that it would be more readable if we remove
completely LOWER_NODE and use only a check based on _MSC_VER for those
two files in ltree? This could also be handled as a separate change.

I'm hesitant to touch that. If anyone is running an instance compiled
with a non-default LOWER_NODE then we might give them some trouble if
they pg_upgrade their database later.

+     foreach my $line (split /\n/, $mf)
+     {
+             if ($line =~ /^[A-Za-z0-9_]*\.o:\s(.*)/)
+             {
+                     foreach my $file (split /\s+/, $1)
+                     {
+                             foreach my $proj (@projects)
+                             {
+                                     $proj->AddFileConditional("$subdir/$n/$file");
+                             }
+                     }
+             }
+     }

Looking closer at this change, I don't think that this is completely
correct and that could become a trap. This is adding quite a bit of
complexity to take care of contrib_extrasource getting empty, and it
actually overlaps with the handling of OBJS done in AddDir(), no?

hmm. I'm not quite sure if I know what you mean by "trap" here.

contrib/cube/Makefile has an example of what this is trying to catch:

# cubescan is compiled as part of cubeparse
cubeparse.o: cubescan.c

I don't really see what other options there are apart from just not
get rid of $contrib_extrasource.

Can you give an example of what sort of scenario you've got in mind
where it'll cause issues?

I've attached a rebased patch.

David

On Wed, 3 Mar 2021 at 22:37, David Rowley <dgrowleyml@gmail.com> wrote:

I've attached a rebased patch.

I've rebased this again.

I also moved away from using hash tables for storing references and
libraries. I was having some problems getting psql to compile due to
the order of the dependencies being reversed due to the order being at
the mercy of Perl's hash function. There's mention of this in
Makefile.global.in:

# libpq_pgport is for use by client executables (not libraries) that use libpq.
# We force clients to pull symbols from the non-shared libraries libpgport
# and libpgcommon rather than pulling some libpgport symbols from libpq just
# because libpq uses those functions too. This makes applications less
# dependent on changes in libpq's usage of pgport (on platforms where we
# don't have symbol export control for libpq). To do this we link to
# pgport before libpq. This does cause duplicate -lpgport's to appear
# on client link lines, since that also appears in $(LIBS).
# libpq_pgport_shlib is the same idea, but for use in client shared libraries.

I switched these back to arrays but added an additional check to only
add new items to the array if we don't already have an element with
the same value.

I've attached the diffs in the *.vcxproj files between patched and unpatched.

David

diff --git a/src/tools/msvc/MSBuildProject.pm b/src/tools/msvc/MSBuildProject.pm
index ebb169e201..68606a296d 100644
--- a/src/tools/msvc/MSBuildProject.pm
+++ b/src/tools/msvc/MSBuildProject.pm
@@ -310,11 +310,12 @@ sub WriteItemDefinitionGroup
my $targetmachine =
$self->{platform} eq 'Win32' ? 'MachineX86' : 'MachineX64';

-	my $includes = $self->{includes};
-	unless ($includes eq '' or $includes =~ /;$/)
+	my $includes = "";
+	foreach my $inc (@{ $self->{includes} })
{
-		$includes .= ';';
+		$includes .= $inc . ";";
}

Perl note: you can do this more easily as

my $includes = join ';', @{$self->{includes}};
$includes .= ';' unless $includes eq '';

--
�lvaro Herrera 39�49'30"S 73�17'W
Are you not unsure you want to delete Firefox?
[Not unsure] [Not not unsure] [Cancel]
http://smylers.hates-software.com/2008/01/03/566e45b2.html

On 4/19/21 12:24 PM, Alvaro Herrera wrote:

diff --git a/src/tools/msvc/MSBuildProject.pm b/src/tools/msvc/MSBuildProject.pm
index ebb169e201..68606a296d 100644
--- a/src/tools/msvc/MSBuildProject.pm
+++ b/src/tools/msvc/MSBuildProject.pm
@@ -310,11 +310,12 @@ sub WriteItemDefinitionGroup
my $targetmachine =
$self->{platform} eq 'Win32' ? 'MachineX86' : 'MachineX64';

-	my $includes = $self->{includes};
-	unless ($includes eq '' or $includes =~ /;$/)
+	my $includes = "";
+	foreach my $inc (@{ $self->{includes} })
{
-		$includes .= ';';
+		$includes .= $inc . ";";
}

Perl note: you can do this more easily as

my $includes = join ';', @{$self->{includes}};
$includes .= ';' unless $includes eq '';

or even more simply:

my $includes = join ';', @{$self->{includes}}, "";

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Tue, 20 Apr 2021 at 09:28, Andrew Dunstan <andrew@dunslane.net> wrote:

On 4/19/21 12:24 PM, Alvaro Herrera wrote:

diff --git a/src/tools/msvc/MSBuildProject.pm b/src/tools/msvc/MSBuildProject.pm
index ebb169e201..68606a296d 100644
--- a/src/tools/msvc/MSBuildProject.pm
+++ b/src/tools/msvc/MSBuildProject.pm
@@ -310,11 +310,12 @@ sub WriteItemDefinitionGroup
my $targetmachine =
$self->{platform} eq 'Win32' ? 'MachineX86' : 'MachineX64';

-    my $includes = $self->{includes};
-    unless ($includes eq '' or $includes =~ /;$/)
+    my $includes = "";
+    foreach my $inc (@{ $self->{includes} })
{
-            $includes .= ';';
+            $includes .= $inc . ";";
}

Perl note: you can do this more easily as

my $includes = join ';', @{$self->{includes}};
$includes .= ';' unless $includes eq '';

or even more simply:

my $includes = join ';', @{$self->{includes}}, "";

Both look more compact. Thanks. I'll include this for the next version.

David

On Mon, Apr 19, 2021 at 5:18 PM David Rowley <dgrowleyml@gmail.com> wrote:

On Wed, 3 Mar 2021 at 22:37, David Rowley <dgrowleyml@gmail.com> wrote:

I've attached a rebased patch.

I've rebased this again.

I also moved away from using hash tables for storing references and
libraries. I was having some problems getting psql to compile due to
the order of the dependencies being reversed due to the order being at
the mercy of Perl's hash function. There's mention of this in
Makefile.global.in:

# libpq_pgport is for use by client executables (not libraries) that use libpq.
# We force clients to pull symbols from the non-shared libraries libpgport
# and libpgcommon rather than pulling some libpgport symbols from libpq just
# because libpq uses those functions too. This makes applications less
# dependent on changes in libpq's usage of pgport (on platforms where we
# don't have symbol export control for libpq). To do this we link to
# pgport before libpq. This does cause duplicate -lpgport's to appear
# on client link lines, since that also appears in $(LIBS).
# libpq_pgport_shlib is the same idea, but for use in client shared libraries.

I switched these back to arrays but added an additional check to only
add new items to the array if we don't already have an element with
the same value.

I've attached the diffs in the *.vcxproj files between patched and unpatched.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

On Thu, 15 Jul 2021 at 04:01, vignesh C <vignesh21@gmail.com> wrote:

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

I've rebased this patch and broken it down into 6 individual patches.

0001: Removes an include directory for dblink. This appears like it's
not needed. It was added in ee3b4188a (Jan 2010), but an earlier
commit, 320c7eb8c (June 2008) seems to have made it pointless. It's
still a mystery to me why ee3b4188a would have been required in the
first place.

0002: Parses -D in the CPPFLAGS of Makefiles and uses those in the
MSVC script. It also adjusts the ltree contrib module so that we do
the same LOWER_NODE behaviour as we did before. The MSVC scripts
appear to have mistakenly forgotten to define LOWER_NODE as it is in
the Makefiles.

0003: Is a tidy up patch to make the 'includes' field an array rather
than a string

0004: Adds code to check for duplicate references and libraries before
adding new ones of the same name to the project.

0005: Is mostly a tidy up so that we use AddFile consistently instead
of sometimes doing $self->{files}->{<name>} = 1;

0006: I'm not so sure about. It attempts to do a bit more Makefile
parsing to get rid of contrib_extrasource and the majority of
contrib_uselibpgport and contrib_uselibpgcommon usages.

David

David Rowley <dgrowleyml@gmail.com> writes:

On Thu, 15 Jul 2021 at 04:01, vignesh C <vignesh21@gmail.com> wrote:

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

I've rebased this patch and broken it down into 6 individual patches.

I don't know anything about the MSVC build process, but I figured I
could do a general Perl code review.

--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm

[…]

+	# Process custom compiler flags
+	if ($mf =~ /^PG_CPPFLAGS\s*=\s*(.*)$/mg || $mf =~ /^override\s*CPPFLAGS\s*(?:[\+\:])?=\s*(.*)$/mg)

^^^^^^^^^^^
This is a very convoluted way of writing [+:]?

--- a/src/tools/msvc/Project.pm
+++ b/src/tools/msvc/Project.pm
@@ -58,7 +58,7 @@ sub AddFiles

while (my $f = shift)
{
-		$self->{files}->{ $dir . "/" . $f } = 1;
+		AddFile($self, $dir . "/" . $f, 1);

AddFile is a method, so should be called as $self->AddFile(…).

--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -36,16 +36,12 @@ my @unlink_on_exit;

[…]

+			elsif ($flag =~ /^-I(.*)$/)
+			{
+				foreach my $proj (@projects)
+				{
+					if ($1 eq '$(libpq_srcdir)')
+					{
+						$proj->AddIncludeDir('src\interfaces\libpq');
+						$proj->AddReference($libpq);
+					}
+				}
+			}

It would be better to do the if check outside the for loop.

--- a/src/tools/msvc/Project.pm
+++ b/src/tools/msvc/Project.pm
@@ -51,6 +51,16 @@ sub AddFile
return;
}

+sub AddFileAndAdditionalFiles
+{
+	my ($self, $filename) = @_;
+	if (FindAndAddAdditionalFiles($self, $filename) != 1)

Again, FindAndAddAdditionalFiles is a method and should be called as
$self->FindAndAddAdditionalFiles($filename).

+	{
+		$self->{files}->{$filename} = 1;
+	}
+	return;
+}

- ilmari

David Rowley <dgrowleyml@gmail.com> writes:

0001: Removes an include directory for dblink. This appears like it's
not needed. It was added in ee3b4188a (Jan 2010), but an earlier
commit, 320c7eb8c (June 2008) seems to have made it pointless. It's
still a mystery to me why ee3b4188a would have been required in the
first place.

FWIW, I poked around in the mailing list archives around that date
and couldn't find any supporting discussion. It does seem like it
shouldn't be needed, given that dblink's Makefile does no such thing.

I'd suggest just pushing your 0001 and seeing if the buildfarm
complains.

0002: Parses -D in the CPPFLAGS of Makefiles and uses those in the
MSVC script. It also adjusts the ltree contrib module so that we do
the same LOWER_NODE behaviour as we did before. The MSVC scripts
appear to have mistakenly forgotten to define LOWER_NODE as it is in
the Makefiles.

The LOWER_NODE situation seems like a mess, but I think the right fix
is to remove -DLOWER_NODE from the Makefile altogether and move the
responsibility into the C code. You could have ltree.h do

#if !defined(_MSC_VER)
#define LOWER_NODE 1
#endif

and put the explanatory comment on that, not on the uses of the flag.

Haven't looked at the rest.

regards, tom lane