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