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

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