Re: Improvement of versioning on Windows, take two

Hi all,

Please find attached a patch finishing the work begun during CF1. This
adds versioning support for all the remaining dll and exe files on
both MinGW and MSVC:
- regress.dll (MSVC only)
- isolationtester.exe
- pg_isolation_regress.exe
- pg_regress.exe
- pg_regress_ecpg.exe
- zic.exe
I will add this patch to CF2. Comments are welcome.

Regards,

Michael

From: "Michael Paquier" <michael(dot)paquier(at)gmail(dot)com>
> Please find attached a patch finishing the work begun during CF1. This
> adds versioning support for all the remaining dll and exe files on
> both MinGW and MSVC:
> - regress.dll (MSVC only)
> - isolationtester.exe
> - pg_isolation_regress.exe
> - pg_regress.exe
> - pg_regress_ecpg.exe
> - zic.exe
> I will add this patch to CF2. Comments are welcome.

The patch applied cleanly to the latest source code. But the build failed
with MSVC 2008 Express due to the exact same LNK1104 error mentioned in:

http://www.postgresql.org/message-id/CAB7nPqRtY1eikQGmz1_wbVHPvFyvE9VMA67ricepQ6D8-EgwnA@mail.gmail.com

LINK : fatal error LNK1104: ファイル
'.\release\postgres\src_timezone_win32ver.obj' を開くことができません。

Regards
MauMau

On Sun, Aug 10, 2014 at 9:31 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> From: "Michael Paquier" <michael(dot)paquier(at)gmail(dot)com>
> LINK : fatal error LNK1104: ファイル
> '.\release\postgres\src_timezone_win32ver.obj' を開くことができません。
Oh yes, right. I don't really know how I missed this error when
testing v1. Adding an explicit call to
RemoveFile('src\timezone\win32ver.rc') for project postgres calms down
the build. Is the attached working for you?
Regards,
--
Michael

From: "Michael Paquier" <michael(dot)paquier(at)gmail(dot)com>
> Oh yes, right. I don't really know how I missed this error when
> testing v1. Adding an explicit call to
> RemoveFile('src\timezone\win32ver.rc') for project postgres calms down
> the build. Is the attached working for you?

Yes, the build succeeded. I confirmed that the following files have version
info. However, unlike other files, they don't have file description. Is
this intended?

bin\isolationtester.exe
bin\pg_isolation_regress
bin\pg_regress.exe
bin\pg_regress_ecpg.exe
bin\zic.exe
lib\regress.dll

lib\dict_snowball.dll has no version properties.

Regards
MauMau

On Tue, Aug 12, 2014 at 11:47 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> From: "Michael Paquier" <michael(dot)paquier(at)gmail(dot)com>
> Yes, the build succeeded. I confirmed that the following files have version
> info. However, unlike other files, they don't have file description. Is
> this intended?
> bin\isolationtester.exe
> bin\pg_isolation_regress
> bin\pg_regress.exe
> bin\pg_regress_ecpg.exe
> bin\zic.exe

No... But after some additional hacking on this, I have been able to
complete that. This has for example required the addition of a new
function called AddUtilityResourceFile in Project.pm that is able to
scan a Makefile within a given folder and to extract PGFILEDESC and
PGAPPICON values that are then used to create a win32ver.rc in the
targetted build folder. Note that on MinGW all those exe/dll had file
description and version number even with previous version.

> lib\regress.dll
With MinGW, this had no file description and no version number. Of
course that was the same with MSVC. Fixed.

> lib\dict_snowball.dll has no version properties.
On MSVC there were no file description and no version number. On
MinGW, I saw a version number. Thanks for spotting that, I've fixed
it.

Regards,
--
Michael

I confirmed that all issues are solved. The patch content looks good,
alghouth I'm not confident in Perl. I marked this patch as ready for
committer. I didn't try the patch on MinGW.

Regards
MauMau

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.

On Tue, Aug 19, 2014 at 12:34 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> Committed after making several fixes, notably:
Thanks a lot, especially for the additional comments.
--
Michael