Re: Reduce the number of special cases to build contrib modules on windows - Mailing list pgsql-hackers

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