Re: Missing file versions for a bunch of dll/exe files in Windows builds

On Fri, Jun 20, 2014 at 9:02 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> From: "Michael Paquier" <michael(dot)paquier(at)gmail(dot)com>
>> Patch 1 should be definitely applied, that's an existing bug. Patch 2
>> and 3 are here to ensure that all the dll/exe files generated have a
>> version number associated with a build on Windows, something
>> particularly useful for upgrades, and important for consistency among
>> files...
> +1 for all these three patches. There seems to be a few issues.
Thanks for taking the time to look at those patches.

> (1)
> The patches applied cleanly, but the build failed. I used MSVC 2008
> Express. build.bat output the following messages at the end. I'm sorry the
> messages are in Japanese; the compiler didn't emit English messages even
> when I switched the code page with chcp.
Thanks, at least I'm fine on this field :)

> "D:\postgresql-9.4\pgsql.sln" (既定のターゲット) (1) ->
> (postgres ターゲット) ->
> LINK : fatal error LNK1104: ファイル
> '.\release\postgres\src_timezone_win32ver.obj' を開くことができません。
>
> 6 警告
> 1 エラー
>
> 経過時間 00:06:19.96
> --------------------------------------------------
> The cause seems to be the following part in postgres.vcproj.
> src\timezone\win32ver.rc entry is present, while it's not without the
> patches.
Interesting to see build failing because of that, this is caused by
the addition of PGFILEDESC in src/timezone/Makefile. Note that I found
something similar with snowball as postgres already includes it,
making a similar conflict with the rc file inclusions. Btw, in the new
patch attached I have removed both of them for the time being, and
build worked fine. This results in dict_snowball.dll not to be
versioned though. At the same time, I am removing as well the
versioning of the regression stuff in src/test/* (isolation, regress)
as they are not mandatory to have for the server and the client
binaries/libs. I also noticed that the dlls of conversion_procs were
not versioned correctly as well, problem fixed in the new patch,

Globally, we could do a better effort in versioning for
dict_snowball.dll, however I'd rather see that in another patch as it
would need some more manipulation of Mkvcbuild.pm & co, and current
patch already improves versioning for a bunch of files already..

> (2)
> The line in contrib/adminpack/Makefile has one extra space after "-". Other
> contribs have one space there.
>
> PGFILEDESC = "adminpack - Support functions for pgAdmin"
Fixed.

> (3)
> Makefiles in contrib/int_agg and contrib/intarray do not have PGFILEDESC.
Fixed. I clearly missed them

> (4)
> Some existing Makefiles should have better description. If you find it
> appropriate to include the improvements in your patch, could you improve the
> description?
>
> * src/bin/pg_basebackup/Makefile has the line:
>
> PGFILEDESC = "pg_basebackup - takes a streaming base backup of a PostgreSQL
> instance"
>
> On the other hand, src/bin/pg_dump/Makefile has:
>
> PGFILEDESC = "pg_dump/pg_restore/pg_dumpall - backup and restore PostgreSQL
> databases"
>
> I think pg_basebackup's Makefile should follow the style of pg_dump, because
> multiple programs are built in pg_basebackup/.
That's an excellent suggestion. Done. This could be done as a separate
patch, master branch and even back branches are missing the shot.

> * contrib/pg_xlogdump/Makefile lacks the command description.
>
> PGFILEDESC = "pg_xlogdump"
Done.

For the sake of the archives, I have been using the following vbscript
to check if a dll/exe has a version number attached:
Set args = WScript.Arguments
Set objFSO = CreateObject("Scripting.FileSystemObject")
Wscript.Echo objFSO.GetFileVersion(args.Item(0))

Then it is only a matter to run it like that:
cscript /nologo my_script.vbs file_to_check.exe

Regards,
--
Michael