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

Thank you for looking at this.

On Tue, 3 Nov 2020 at 09:49, Andres Freund <andres(at)anarazel(dot)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