Re: 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