Re: Improvement of versioning on Windows, take two - Mailing list pgsql-hackers

Committed after making several fixes, notably:

On Thu, Aug 14, 2014 at 03:59:57PM +0900, Michael Paquier wrote:
> --- a/src/test/isolation/Makefile
> +++ b/src/test/isolation/Makefile
> @@ -6,12 +6,15 @@ subdir = src/test/isolation
>  top_builddir = ../../..
>  include $(top_builddir)/src/Makefile.global
>  
> +PGFILEDESC = "pg_isolation_tester/isolationtester - isolation regression tests"

Typo.

> +PGAPPICON = win32

In the MinGW build, PGAPPICON is ineffective unless set before including
Makefile.global.

> --- a/src/tools/msvc/Mkvcbuild.pm
> +++ b/src/tools/msvc/Mkvcbuild.pm

> @@ -132,6 +133,8 @@ sub mkvcbuild
>              return shift !~ /dict_snowball.c$/;
>          });
>      $snowball->AddIncludeDir('src\include\snowball');
> +    $snowball->AddUtilityResourceFile('src\backend\snowball');
> +    $snowball->RemoveFile('src\backend\snowball\libstemmer\win32ver.rc');

Augmenting the RelocateFiles callback seemed much more natural.

> @@ -597,6 +605,7 @@ sub mkvcbuild
>      $pgregress->AddFile('src\test\regress\pg_regress_main.c');
>      $pgregress->AddIncludeDir('src\port');
>      $pgregress->AddDefine('HOST_TUPLE="i686-pc-win32vc"');
> +    $pgregress->AddResourceFile('src\test\regress');

Wrong method name.  (This failed to fail, because the previous project
initialized win32ver.rc in the same directory.)

> --- a/src/tools/msvc/Project.pm
> +++ b/src/tools/msvc/Project.pm
> @@ -303,6 +303,46 @@ sub AddDir
>      $/ = $t;
>  }
>  
> +sub AddUtilityResourceFile
> +{

This function scans a directory's Makefile for PGFILEDESC and PGAPPICON,
passing their content to AddResourceFile().  Compare AddDir(), which scans for
those things and a few more (OBJS, SUBDIRS, etc.).  This function also raises
an error if the directory does not have both settings.  In that light, the
word "Utility" didn't accurately set this function apart from its peers.  One
uses this function when four-arg AddProject() is inapplicable.  (For example,
when the directory yields multiple .exe and/or .dll products.)  Whether the
directory builds, in some sense, a "utility" is irrelevant.  Also, let's not
have two copies of the code for extracting PGFILEDESC and PGAPPICON.  I
renamed this to AddDirResourceFile(), extracted the AddDir() implementation,
and made AddDir() call this.

It would be nice to break the build when someone adds a (non-whitelisted)
project that omits version information or the icon.  New violators were
unlikely to be direct AddDirResourceFile() callers, though.

> +    open($MF, "$dir\\GNUMakefile")
> +      || open($MF, "$dir\\Makefile")
> +      || croak "Could not open $dir\\Makefile\n";

You swapped the filename preference order compared to AddDir().  Adding that
sort of inconsistency called for a comment.  Since this order is strictly
better, I instead standardized on it.

> +    print 'Directory: ' . $dir . "\n\r";

Leftover debugging code.

> --- a/src/tools/msvc/clean.bat
> +++ b/src/tools/msvc/clean.bat
> @@ -20,10 +20,16 @@ del /s /q src\bin\win32ver.rc 2> NUL
>  del /s /q src\interfaces\win32ver.rc 2> NUL
>  if exist src\backend\win32ver.rc del /q src\backend\win32ver.rc
>  if exist src\backend\replication\libpqwalreceiver\win32ver.rc del /q
src\backend\replication\libpqwalreceiver\win32ver.rc
> +if exist src\backend\snowball\libstemmer\win32ver.rc del /q src\backend\snowball\libstemmer\win32ver.rc

That's not the location of the snowball resource file.