Thank you for looking at this.
On Tue, 3 Nov 2020 at 09:49, Andres Freund <and...@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
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 90594bd41b..f8ab5f59f5 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 = {};
+my @contrib_uselibpq = ();
+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 = {};
my @contrib_excludes = (
'bool_plperl', 'commit_ts',
'hstore_plperl', 'hstore_plpython',
@@ -951,6 +948,7 @@ sub AddContrib
my $subdir = shift;
my $n = shift;
my $mf = Project::read_file("$subdir/$n/Makefile");
+ my @projects;
if ($mf =~ /^MODULE_big\s*=\s*(.*)$/mg)
{
@@ -958,6 +956,7 @@ sub AddContrib
my $proj = $solution->AddProject($dn, 'dll', 'contrib',
"$subdir/$n");
$proj->AddReference($postgres);
AdjustContribProj($proj);
+ push @projects, $proj;
}
elsif ($mf =~ /^MODULES\s*=\s*(.*)$/mg)
{
@@ -969,18 +968,91 @@ sub AddContrib
$proj->AddFile("$subdir/$n/$filename");
$proj->AddReference($postgres);
AdjustContribProj($proj);
+ push @projects, $proj;
}
}
elsif ($mf =~ /^PROGRAM\s*=\s*(.*)$/mg)
{
my $proj = $solution->AddProject($1, 'exe', 'contrib',
"$subdir/$n");
AdjustContribProj($proj);
+ push @projects, $proj;
}
else
{
croak "Could not determine contrib module type for $n\n";
}
+ # Process custom compiler flags
+ if ($mf =~ /^PG_CPPFLAGS\s*=\s*(.*)$/mg)
+ {
+ 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);
+ }
+ }
+ }
+ }
+ }
+
+ if ($mf =~ /^SHLIB_LINK_INTERNAL\s*=\s*(.*)$/mg)
+ {
+ foreach my $lib (split /\s+/, $1)
+ {
+ if ($lib eq '$(libpq)')
+ {
+ foreach my $proj (@projects)
+ {
+
$proj->AddIncludeDir('src\interfaces\libpq');
+ $proj->AddReference($libpq);
+ }
+ }
+ }
+ }
+
+ if ($mf =~ /^PG_LIBS_INTERNAL\s*=\s*(.*)$/mg)
+ {
+ foreach my $lib (split /\s+/, $1)
+ {
+ if ($lib eq '$(libpq_pgport)')
+ {
+ foreach my $proj (@projects)
+ {
+ $proj->AddReference($libpgport);
+ $proj->AddReference($libpgcommon);
+ }
+ }
+ }
+ }
+
+ 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");
+ }
+ }
+
+ }
+ }
+
# Are there any output data files to build?
GenerateContribSqlFiles($n, $mf);
return;
diff --git a/src/tools/msvc/Project.pm b/src/tools/msvc/Project.pm
index 20f79b382b..3827e9e334 100644
--- a/src/tools/msvc/Project.pm
+++ b/src/tools/msvc/Project.pm
@@ -42,12 +42,25 @@ sub _new
sub AddFile
{
- my ($self, $filename) = @_;
+ my ($self, $filename, $alwaysAddOriginal) = @_;
+ FindAndAddAdditionalFiles($self, $filename);
+ # Add the original file regardless to the return value of
+ # FindAndAddAdditionalFiles
$self->{files}->{$filename} = 1;
return;
}
+sub AddFileConditional
+{
+ my ($self, $filename) = @_;
+ if (FindAndAddAdditionalFiles($self, $filename) != 1)
+ {
+ $self->{files}->{$filename} = 1;
+ }
+ return;
+}
+
sub AddFiles
{
my $self = shift;
@@ -55,11 +68,40 @@ sub AddFiles
while (my $f = shift)
{
- $self->{files}->{ $dir . "/" . $f } = 1;
+ AddFile($self, $dir . "/" . $f, 1);
}
return;
}
+# 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{\.[^.]+$}{};
+ if (-e "$filenoext.l")
+ {
+ AddFileConditional($self, "$filenoext.l");
+ return 1;
+ }
+ if (-e "$filenoext.y")
+ {
+ AddFileConditional($self, "$filenoext.y");
+ return 0;
+ }
+ }
+ return 0;
+}
+
sub ReplaceFile
{
my ($self, $filename, $newname) = @_;
@@ -74,14 +116,14 @@ sub ReplaceFile
if ($file eq $filename)
{
delete $self->{files}{$file};
- $self->{files}{$newname} = 1;
+ AddFile($self, $newname);
return;
}
}
elsif ($file =~ m/($re)/)
{
delete $self->{files}{$file};
- $self->{files}{"$newname/$filename"} = 1;
+ AddFile($self, "$newname/$filename");
return;
}
}
@@ -256,11 +298,11 @@ sub AddDir
if ($f =~ /^\$\(top_builddir\)\/(.*)/)
{
$f = $1;
- $self->{files}->{$f} = 1;
+ AddFile($self, $f);
}
else
{
- $self->{files}->{"$reldir/$f"} = 1;
+ AddFile($self, "$reldir/$f");
}
}
$mf =~ s{OBJS[^=]*=\s*(.*)$}{}m;
