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

Hi all,

A colleague of mine pointed out that the file version is missing in a
couple of dll and exe files when building on windows using the
community scripts in src/tools/msvc. After having a closer look, I
noticed that a *lot* of files are missing the shot:
- all the exe/dll in contrib/
- dll of PL languages (perl, python, tcl, pgsql)
- libpqwalreceiver, snowball
- ecpg stuff
- regression and isolation test stuff
- conversion_procs thingies
Having a version number associated to a build is important for all
companies creating builds of Postgres on Windows, so it would be good
to have the patch attached applied and back-patched.

Regards,
--
Michael

On Thu, Apr 24, 2014 at 7:54 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com>wrote:

> Hi all,
>
> A colleague of mine pointed out that the file version is missing in a
> couple of dll and exe files when building on windows using the
> community scripts in src/tools/msvc. After having a closer look, I
> noticed that a *lot* of files are missing the shot:
> - all the exe/dll in contrib/
> - dll of PL languages (perl, python, tcl, pgsql)
> - libpqwalreceiver, snowball
> - ecpg stuff
> - regression and isolation test stuff
> - conversion_procs thingies
> Having a version number associated to a build is important for all
> companies creating builds of Postgres on Windows, so it would be good
> to have the patch attached applied and back-patched.
>
>
At least some fo that is intentional - things that are considered
"internal" were not given a version resource intentionally. E.g. the
conversion_procs is very intentional. The EXE/DLL in contrib should
definitely have them though.

it also seems like the wrong way to go about it - for all the other files,
it's added by rule (when PGFILEDESC is specified in the Makefile). Which
currently appears to be the *only* way it's added, so are you saying this
just doesn't work? Or does it work for some of them?

I think the proper solutioni s to add PGFILEDESC entries to the Makefile's,
and if that one doesn't actually work then fix the build system to work :)
(Sorry, don't have a win32 build environment around to test it right now)

Then it will be consistent between mingw and msvc, which your patch isn't,
I believe?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Thu, Apr 24, 2014 at 8:27 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Thu, Apr 24, 2014 at 7:54 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
> wrote:
>>
>> Hi all,
>>
>> A colleague of mine pointed out that the file version is missing in a
>> couple of dll and exe files when building on windows using the
>> community scripts in src/tools/msvc. After having a closer look, I
>> noticed that a *lot* of files are missing the shot:
>> - all the exe/dll in contrib/
>> - dll of PL languages (perl, python, tcl, pgsql)
>> - libpqwalreceiver, snowball
>> - ecpg stuff
>> - regression and isolation test stuff
>> - conversion_procs thingies
>> Having a version number associated to a build is important for all
>> companies creating builds of Postgres on Windows, so it would be good
>> to have the patch attached applied and back-patched.
>>
>
> At least some fo that is intentional - things that are considered "internal"
> were not given a version resource intentionally. E.g. the conversion_procs
> is very intentional.

Why? Version resources are kinda handy for installers to reliably
figure out whether or not something should be upgraded.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Apr 24, 2014 at 9:50 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> On Thu, Apr 24, 2014 at 8:27 AM, Magnus Hagander <magnus(at)hagander(dot)net>
> wrote:
> > On Thu, Apr 24, 2014 at 7:54 AM, Michael Paquier <
> michael(dot)paquier(at)gmail(dot)com>
> > wrote:
> >>
> >> Hi all,
> >>
> >> A colleague of mine pointed out that the file version is missing in a
> >> couple of dll and exe files when building on windows using the
> >> community scripts in src/tools/msvc. After having a closer look, I
> >> noticed that a *lot* of files are missing the shot:
> >> - all the exe/dll in contrib/
> >> - dll of PL languages (perl, python, tcl, pgsql)
> >> - libpqwalreceiver, snowball
> >> - ecpg stuff
> >> - regression and isolation test stuff
> >> - conversion_procs thingies
> >> Having a version number associated to a build is important for all
> >> companies creating builds of Postgres on Windows, so it would be good
> >> to have the patch attached applied and back-patched.
> >>
> >
> > At least some fo that is intentional - things that are considered
> "internal"
> > were not given a version resource intentionally. E.g. the
> conversion_procs
> > is very intentional.
>
> Why? Version resources are kinda handy for installers to reliably
> figure out whether or not something should be upgraded.
>

I think the general argument was "maintenance overhead". This was back
before the 8.0 release, so my memory is somewhat sketchy :)

Which also means of course that we can reconsider that decision. But if we
do, we need to support it equally in mingw and msvc.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Thu, Apr 24, 2014 at 9:13 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Thu, Apr 24, 2014 at 9:50 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>> On Thu, Apr 24, 2014 at 8:27 AM, Magnus Hagander <magnus(at)hagander(dot)net>
>> wrote:
>> > On Thu, Apr 24, 2014 at 7:54 AM, Michael Paquier
>> > <michael(dot)paquier(at)gmail(dot)com>
>> > wrote:
>> >>
>> >> Hi all,
>> >>
>> >> A colleague of mine pointed out that the file version is missing in a
>> >> couple of dll and exe files when building on windows using the
>> >> community scripts in src/tools/msvc. After having a closer look, I
>> >> noticed that a *lot* of files are missing the shot:
>> >> - all the exe/dll in contrib/
>> >> - dll of PL languages (perl, python, tcl, pgsql)
>> >> - libpqwalreceiver, snowball
>> >> - ecpg stuff
>> >> - regression and isolation test stuff
>> >> - conversion_procs thingies
>> >> Having a version number associated to a build is important for all
>> >> companies creating builds of Postgres on Windows, so it would be good
>> >> to have the patch attached applied and back-patched.
>> >>
>> >
>> > At least some fo that is intentional - things that are considered
>> > "internal"
>> > were not given a version resource intentionally. E.g. the
>> > conversion_procs
>> > is very intentional.
>>
>> Why? Version resources are kinda handy for installers to reliably
>> figure out whether or not something should be upgraded.
>
>
> I think the general argument was "maintenance overhead". This was back
> before the 8.0 release, so my memory is somewhat sketchy :)
>
> Which also means of course that we can reconsider that decision. But if we
> do, we need to support it equally in mingw and msvc.

Well it's pretty darn close to zero maintenance isn't it? We don't
even have to maintain rc files with the proposed patch (if my
extremely weak perl-fu is reading the code correctly).

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Apr 24, 2014 at 10:18 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> On Thu, Apr 24, 2014 at 9:13 AM, Magnus Hagander <magnus(at)hagander(dot)net>
> wrote:
> > On Thu, Apr 24, 2014 at 9:50 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> >>
> >> On Thu, Apr 24, 2014 at 8:27 AM, Magnus Hagander <magnus(at)hagander(dot)net>
> >> wrote:
> >> > On Thu, Apr 24, 2014 at 7:54 AM, Michael Paquier
> >> > <michael(dot)paquier(at)gmail(dot)com>
> >> > wrote:
> >> >>
> >> >> Hi all,
> >> >>
> >> >> A colleague of mine pointed out that the file version is missing in a
> >> >> couple of dll and exe files when building on windows using the
> >> >> community scripts in src/tools/msvc. After having a closer look, I
> >> >> noticed that a *lot* of files are missing the shot:
> >> >> - all the exe/dll in contrib/
> >> >> - dll of PL languages (perl, python, tcl, pgsql)
> >> >> - libpqwalreceiver, snowball
> >> >> - ecpg stuff
> >> >> - regression and isolation test stuff
> >> >> - conversion_procs thingies
> >> >> Having a version number associated to a build is important for all
> >> >> companies creating builds of Postgres on Windows, so it would be good
> >> >> to have the patch attached applied and back-patched.
> >> >>
> >> >
> >> > At least some fo that is intentional - things that are considered
> >> > "internal"
> >> > were not given a version resource intentionally. E.g. the
> >> > conversion_procs
> >> > is very intentional.
> >>
> >> Why? Version resources are kinda handy for installers to reliably
> >> figure out whether or not something should be upgraded.
> >
> >
> > I think the general argument was "maintenance overhead". This was back
> > before the 8.0 release, so my memory is somewhat sketchy :)
> >
> > Which also means of course that we can reconsider that decision. But if
> we
> > do, we need to support it equally in mingw and msvc.
>
> Well it's pretty darn close to zero maintenance isn't it? We don't
> even have to maintain rc files with the proposed patch (if my
> extremely weak perl-fu is reading the code correctly).
>
>
Probably, yes. It may just have been the caution of the first release..

That said, as also noted, the patch doesn't actually fix the problem :P For
one it establishes two different ways of generating the resources. And it
also doesn't support mingw. So it's likely going to be a little bit more
involved, but probably not hugely much. And if we do it right, ongoing
maintenance should hopefully be very low.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Thu, Apr 24, 2014 at 4:27 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> At least some fo that is intentional - things that are considered "internal"
> were not given a version resource intentionally. E.g. the conversion_procs
> is very intentional. The EXE/DLL in contrib should definitely have them
> though.
Why isn't conversion_procs done? Just to lower the maintenance pain?

> it also seems like the wrong way to go about it - for all the other files,
> it's added by rule (when PGFILEDESC is specified in the Makefile). Which
> currently appears to be the *only* way it's added, so are you saying this
> just doesn't work? Or does it work for some of them?
PGFILEDESC gets recognized, but not for any Makefile in contrib/. I am
guessing that a call to AddDir is missing when defining the contrib
projects. Just a guess from reading the code.

> I think the proper solutioni s to add PGFILEDESC entries to the Makefile's,
> and if that one doesn't actually work then fix the build system to work :)
> (Sorry, don't have a win32 build environment around to test it right now)
Yeah, that's what I thought, until I noticed that PGFILEDESC is only
defined in Makefile of contrib modules containing binaries
(pg_upgrade, oid2name, etc.). There is no documentation describing
this variable except what I could find in some archives of 2004.

> Then it will be consistent between mingw and msvc, which your patch isn't, I
> believe?
There is no reference in the source code to mingw. Am I missing smth?
--
Michael

On Thu, Apr 24, 2014 at 6:00 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> That said, as also noted, the patch doesn't actually fix the problem :P For
> one it establishes two different ways of generating the resources.
I still lack knowledge of those scripts. Any pointers on how to do
that more correctly?
--
Michael

On Thu, Apr 24, 2014 at 11:09 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com
> wrote:

> On Thu, Apr 24, 2014 at 4:27 PM, Magnus Hagander <magnus(at)hagander(dot)net>
> wrote:
> > At least some fo that is intentional - things that are considered
> "internal"
> > were not given a version resource intentionally. E.g. the
> conversion_procs
> > is very intentional. The EXE/DLL in contrib should definitely have them
> > though.
> Why isn't conversion_procs done? Just to lower the maintenance pain?
>
>
IIRC, yes.

> > it also seems like the wrong way to go about it - for all the other
> files,
> > it's added by rule (when PGFILEDESC is specified in the Makefile). Which
> > currently appears to be the *only* way it's added, so are you saying this
> > just doesn't work? Or does it work for some of them?
> PGFILEDESC gets recognized, but not for any Makefile in contrib/. I am
> guessing that a call to AddDir is missing when defining the contrib
> projects. Just a guess from reading the code.
>

Quite possibly so - in which case that's probably what should be fixed,
rather than adding manual calls all over the place.

It does have it for pgevent.dll, btw - that's the one that also uses
PGFILESHLIB which would have to be set properly for anything else that's a
DLL.

> I think the proper solutioni s to add PGFILEDESC entries to the
> Makefile's,
> > and if that one doesn't actually work then fix the build system to work
> :)
> > (Sorry, don't have a win32 build environment around to test it right now)
> Yeah, that's what I thought, until I noticed that PGFILEDESC is only
> defined in Makefile of contrib modules containing binaries
> (pg_upgrade, oid2name, etc.). There is no documentation describing
> this variable except what I could find in some archives of 2004.
>

Hehe, yeah. The patch should add it to the others that need the version
resource.

> > Then it will be consistent between mingw and msvc, which your patch
> isn't, I
> > believe?
> There is no reference in the source code to mingw. Am I missing smth?
>

mingw builds using the Makefiles, not the "perl based custom build system".
In particular, the PGFILEDESC stuff s in makefiles/Makefile.win32. (Also
PGFILESHLIB)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Thu, Apr 24, 2014 at 6:18 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> Quite possibly so - in which case that's probably what should be fixed,
> rather than adding manual calls all over the place.
> It does have it for pgevent.dll, btw - that's the one that also uses
> PGFILESHLIB which would have to be set properly for anything else that's a
> DLL.
OK, fine for me. Including the makefiles of PL languages and of all
the other modules, there would be smth like 30~40 files touched, a
number pretty high for a change that should be back-patched IMO as it
impacts existing versions (I'd actually need the fix for 9.3
precisely, or I would have to go with a hack similar to what I posted
earlier)...

>> > Then it will be consistent between mingw and msvc, which your patch
>> > isn't, I
>> > believe?
>> There is no reference in the source code to mingw. Am I missing smth?
>
>
> mingw builds using the Makefiles, not the "perl based custom build system".
> In particular, the PGFILEDESC stuff s in makefiles/Makefile.win32. (Also
> PGFILESHLIB)
Oh OK. I'll try to get a clean patch following those lines. There is
as well the AddDir call for contrib/ to add...

Regards,
--
Michael

On Thu, Apr 24, 2014 at 8:31 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> Oh OK. I'll try to get a clean patch following those lines. There is
> as well the AddDir call for contrib/ to add...
I went through those things, resulting in the following set of patches:
- 20140425_msvc_adddir.patch, adding some AddDir for contrib modules
to be able to detect PGFILEDESC and generate resource files in
consequence.
- 20140425_pgfiledesc_contrib.patch, adding PGFILEDESC to all the
modules that actually need it (PL things, snowball, contrib/, etc.)
- 20140425_pgfiledesc_cvprocs.patch, adding PGFILEDESC to all the
modules in conversion_procs. OK this does not ease the maintenance
pain, but for correctness in the file version of a build this is
important.

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...
Regards,
--
Michael

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.

(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.

--------------------------------------------------
...
cl : コマンド ライン warning D9024: ソースファイルの種類
'.\src\timezone\win32ver.rc' は認識できませんでした。オブジェクト ファイルと仮定します。
cl : コマンド ライン warning D9027: ソースファイル
'.\src\timezone\win32ver.rc' は無視されます。
cl : コマンド ライン warning D9021: 何も実行されませんでした

"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.

--------------------------------------------------
<File RelativePath="src\timezone\strftime.c" />
<File RelativePath="src\timezone\win32ver.rc"><FileConfiguration
Name="Debug|Win32"><Tool Name="VCCLCompilerTool"
ObjectFile=".\debug\postgres\src_timezone_win32ver.obj"
/></FileConfiguration><FileConfiguration Name="Release|Win32"><Tool
Name="VCCLCompilerTool"
ObjectFile=".\release\postgres\src_timezone_win32ver.obj"
/></FileConfiguration></File>
</Filter>
</Filter>
</Files>
<Globals/>
</VisualStudioProject>
--------------------------------------------------

(2)
The line in contrib/adminpack/Makefile has one extra space after "-". Other
contribs have one space there.

PGFILEDESC = "adminpack - Support functions for pgAdmin"

(3)
Makefiles in contrib/int_agg and contrib/intarray do not have PGFILEDESC.

(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/.

* contrib/pg_xlogdump/Makefile lacks the command description.

PGFILEDESC = "pg_xlogdump"

Regards
MauMau

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

From: "Michael Paquier" <michael(dot)paquier(at)gmail(dot)com>
> 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,

I see, but that's mottainai. I could remove the build errors regarding
src/timezone and dict_snowball by removing the .rc file as follows. pgevent
already seems to do something similar. This gets the patch closer to
completeness. I didn't try the stuff in src/test/.

$postgres->AddDir('src\timezone');
$postgres->RemoveFile('src\timezone\win32ver.rc');
$postgres->AddFiles('src\backend\parser', 'scan.l', 'gram.y');

$snowball->AddReference($postgres);
$snowball->RemoveFile('src\backend\snowball\libstemmer\win32ver.rc');

> 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..

Yes, your current patch improved the file versioning greatly, and I'm
comfortable with it. Please tell me if you fix src/timezone, dict_snowball
and src/test stuff. I'm okay with either. Then I'll continue the test with
the current or next patch.

Regards
MauMau

On Sat, Jun 21, 2014 at 1:44 AM, MauMau <maumau307(at)gmail(dot)com> wrote:
> From: "Michael Paquier" <michael(dot)paquier(at)gmail(dot)com>
>
>> 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,
>
>
> I see, but that's mottainai. I could remove the build errors regarding
> src/timezone and dict_snowball by removing the .rc file as follows. pgevent
> already seems to do something similar. This gets the patch closer to
> completeness. I didn't try the stuff in src/test/.
>
> $postgres->AddDir('src\timezone');
> $postgres->RemoveFile('src\timezone\win32ver.rc');
> $postgres->AddFiles('src\backend\parser', 'scan.l', 'gram.y');
>
> $snowball->AddReference($postgres);
> $snowball->RemoveFile('src\backend\snowball\libstemmer\win32ver.rc');
Yeah this trick is fine as well, but it complicates the code without
versioning those paths :) So let's do without please and find a
cleaner solution later on.

>> 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..
>
>
> Yes, your current patch improved the file versioning greatly, and I'm
> comfortable with it. Please tell me if you fix src/timezone, dict_snowball
> and src/test stuff. I'm okay with either. Then I'll continue the test with
> the current or next patch.
Current patch would be better, conversion_procs paths are now
correctly versioned.
--
Michael

From: "Michael Paquier" <michael(dot)paquier(at)gmail(dot)com>
> Yeah this trick is fine as well, but it complicates the code without
> versioning those paths :) So let's do without please and find a
> cleaner solution later on.

OK.

> Current patch would be better, conversion_procs paths are now
> correctly versioned.

OK, I tested with the current patches:

0001-Add-AddDir-calls-for-contrib.patch
0002-Add-file-versioning-for-all-core-dll-and-exe-in-MSVC.patch

They applied cleanly, and the build with MSVC succeeded.

I checked if Exes and DLLs in bin\ and lib\ have proper versioning info by
looking at the file property with Windows Explorer. I could confirm that
most contrib modules and plpgsql.dll have correct versioning ifno. The
following modules have better description now. Thanks.

bin\pg_basebackup.exe
bin\pg_receivexlog.exe
bin\pg_recvlogical.exe
bin\pg_xlogdump.exe

The below modules did not have versioning info as intended. I hope this
will be improved in the future.

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

However, there seems to be two issues before marking this patch as ready for
committer.

(1)
lib\pgcrypto.dll doesn't have versioning info.

(2)
None of the conversion_procs has versioning info. But their Makefiles have
PGFILEDESC line. For example,
src/backend/utils/mb/conversion_procs/ascii_and_mic/Makefile has the line:

NAME = ascii_and_mic
PGFILEDESC = "ascii_and_mic"

Regards
MauMau

On Sat, Jun 21, 2014 at 11:47 AM, MauMau <maumau307(at)gmail(dot)com> wrote:
> bin\isolationtester.exe
> bin\pg_isolation_regress
> bin\pg_regress.exe
> bin\pg_regress_ecpg.exe
> bin\zic.exe
> lib\dict_snowball.dll
> lib\regress.dll
Yes, I'd like to improve that in a next patch hopefully in CF2, except
if a committer picking up this patch thinks differently.

> However, there seems to be two issues before marking this patch as ready for
> committer.
> (1)
> lib\pgcrypto.dll doesn't have versioning info.
Urgh, thanks I missed it. It is fixed in the patch attached by adding
a AddDir call in the project of pgcrypto as this contrib module is
kind of particular in the MSVC scripts. Here is the output of my
cscript checking versioning with the new patch:
$ cscript ..\extra\vb_version.vbs lib\pgcrypto.dll
Microsoft (R) Windows Script Host Version 5.8
Copyright (C) Microsoft Corporation. All rights reserved.

9.5.0.14171

> (2)
> None of the conversion_procs has versioning info. But their Makefiles have
> PGFILEDESC line. For example,
> src/backend/utils/mb/conversion_procs/ascii_and_mic/Makefile has the line:
>
> NAME = ascii_and_mic
> PGFILEDESC = "ascii_and_mic"
Strange, it works for me:
$ cscript ..\extra\vb_version.vbs lib\utf8_and_ascii.dll

Microsoft (R) Windows Script Host Version 5.8
Copyright (C) Microsoft Corporation. All rights reserved.

9.5.0.14170
Regards,
--
Michael

From: "Michael Paquier" <michael(dot)paquier(at)gmail(dot)com>
> On Sat, Jun 21, 2014 at 11:47 AM, MauMau <maumau307(at)gmail(dot)com> wrote:
>> (1)
>> lib\pgcrypto.dll doesn't have versioning info.
> Urgh, thanks I missed it. It is fixed in the patch attached by adding
> a AddDir call in the project of pgcrypto as this contrib module is
> kind of particular in the MSVC scripts. Here is the output of my
> cscript checking versioning with the new patch:

Thanks, I confirmed that pgcrypto.dll has proper versioning info.

>> (2)
>> None of the conversion_procs has versioning info. But their Makefiles
>> have
>> PGFILEDESC line. For example,
>> src/backend/utils/mb/conversion_procs/ascii_and_mic/Makefile has the
>> line:
>>
>> NAME = ascii_and_mic
>> PGFILEDESC = "ascii_and_mic"
> Strange, it works for me:

Sorry, this was my mistake. The patch application of Mkvcbuild.pm in
0002*.patch failed due to file permissions. conversion_procs Dlls now have
correct versioning info.

I marked this CommitFest entry as ready for committer.

Regards
MauMau

I started editing these patches for commit, but I ran into two defects.
First, clean.bat requires an update to remove the new win32ver.rc files.
Second, the MinGW build uses few or none of the new PGFILEDESC entries; you
need to mention $(WIN32RES) as in (most) existing PGFILEDESC-using makefiles.

Here are the patches as they stood at the time of those discoveries.
Project::AddDir() does more than look for PGFILEDESC. Your patches placed
AddDir calls next to code made redundant by those calls; I removed the
now-redundant code. I prefer four-argument Solution::AddProject() over
three-argument Solution::AddProject() followed promptly by Project::AddDir(),
but that was a mere cosmetic judgement. Our prevailing style is to not
uppercase the letter after the hyphen in PGFILEDESC, and I edited PGFILEDESC
wording here and there. Please update these to fix the two bugs noted above.

You requested a back-patch, but no part of this change qualifies. Even given
a back-patch, automata checking this metadata should point at one of the files
that already has it.

On Sat, Jun 21, 2014 at 10:13:40PM +0900, Michael Paquier wrote:
> On Sat, Jun 21, 2014 at 11:47 AM, MauMau <maumau307(at)gmail(dot)com> wrote:
> > bin\isolationtester.exe
> > bin\pg_isolation_regress
> > bin\pg_regress.exe
> > bin\pg_regress_ecpg.exe
> > bin\zic.exe
> > lib\dict_snowball.dll
> > lib\regress.dll
> Yes, I'd like to improve that in a next patch hopefully in CF2, except
> if a committer picking up this patch thinks differently.

While I'd have preferred to see all binaries covered at once, that's fine.

nm

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

On Fri, Jul 11, 2014 at 12:02 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> I started editing these patches for commit, but I ran into two defects.
Thanks for looking at that and helping out!

> First, clean.bat requires an update to remove the new win32ver.rc files.
Done. The removal of the additional rc files is grouped at the
beginning of clean.bat.

> Second, the MinGW build uses few or none of the new PGFILEDESC entries; you
> need to mention $(WIN32RES) as in (most) existing PGFILEDESC-using makefiles.
I see, that's specified in src/makefiles/Makefile.win32. For the
portion of conversion_procs, updating directly proc.mk seems to be
enough, so done this way. Also, an entry in conversion_procs was not
updated according to the PGFILEDESC you introduced.

Btw, looking more at those patches, I found a limitation with
contrib/spi for MinGW builds: in order to pass WIN32RES to OBJS, some
of the contrib defined as MODULES need to be defined as MODULE_big to
accept a custom list of OBJS, but MODULE_big is not able to accept a
list of shared libraries. This limitation causes all the dll generated
in contrib/spi to not be versioned with MinGW. MSVC works fine though.

> Here are the patches as they stood at the time of those discoveries.
> Project::AddDir() does more than look for PGFILEDESC. Your patches placed
> AddDir calls next to code made redundant by those calls; I removed the
> now-redundant code. I prefer four-argument Solution::AddProject() over
> three-argument Solution::AddProject() followed promptly by Project::AddDir(),
> but that was a mere cosmetic judgement. Our prevailing style is to not
> uppercase the letter after the hyphen in PGFILEDESC, and I edited PGFILEDESC
> wording here and there. Please update these to fix the two bugs noted above.
Ah, right. Nice catch.

> On Sat, Jun 21, 2014 at 10:13:40PM +0900, Michael Paquier wrote:
>> On Sat, Jun 21, 2014 at 11:47 AM, MauMau <maumau307(at)gmail(dot)com> wrote:
>> > bin\isolationtester.exe
>> > bin\pg_isolation_regress
>> > bin\pg_regress.exe
>> > bin\pg_regress_ecpg.exe
>> > bin\zic.exe
>> > lib\dict_snowball.dll
>> > lib\regress.dll
>> Yes, I'd like to improve that in a next patch hopefully in CF2, except
>> if a committer picking up this patch thinks differently.
>
> While I'd have preferred to see all binaries covered at once, that's fine.
Thanks, I am still planning to work more on those things for CF2 and
next commit fests.

Updated patches are attached.

Regards,
--
Michael

On Fri, Jul 11, 2014 at 01:21:57PM +0900, Michael Paquier wrote:
> On Fri, Jul 11, 2014 at 12:02 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > Second, the MinGW build uses few or none of the new PGFILEDESC entries; you
> > need to mention $(WIN32RES) as in (most) existing PGFILEDESC-using makefiles.
> I see, that's specified in src/makefiles/Makefile.win32. For the
> portion of conpversion_procs, updating directly proc.mk seems to be
> enough, so done this way. Also, an entry in conversion_procs was not
> updated according to the PGFILEDESC you introduced.
>
> Btw, looking more at those patches, I found a limitation with
> contrib/spi for MinGW builds: in order to pass WIN32RES to OBJS, some
> of the contrib defined as MODULES need to be defined as MODULE_big to
> accept a custom list of OBJS, but MODULE_big is not able to accept a
> list of shared libraries. This limitation causes all the dll generated
> in contrib/spi to not be versioned with MinGW. MSVC works fine though.

Please fix the MODULES case, perhaps by linking $(WIN32RES) into each library
implicitly.

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

On Fri, Jul 11, 2014 at 10:34 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> Please fix the MODULES case, perhaps by linking $(WIN32RES) into each library
> implicitly.
Yes, that's exactly what I did in the patch attached by linking
WIN32RES and a shared library when the lib is built from a single .o
file. The rule in src/makefiles/Makefile.win32 has been updated
accordingly. pgxs.mk has been updated as well to remove WIN32RES when
clean is invoked for MODULES.
--
Michael

On Fri, Jul 11, 2014 at 11:14:54PM -0700, Michael Paquier wrote:
> On Fri, Jul 11, 2014 at 10:34 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > Please fix the MODULES case, perhaps by linking $(WIN32RES) into each library
> > implicitly.
> Yes, that's exactly what I did in the patch attached by linking
> WIN32RES and a shared library when the lib is built from a single .o
> file. The rule in src/makefiles/Makefile.win32 has been updated
> accordingly. pgxs.mk has been updated as well to remove WIN32RES when
> clean is invoked for MODULES.

There remained several binaries versioned under MSVC and not versioned under
MinGW. With those fixed, I committed this work.

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

On Tue, Jul 15, 2014 at 4:03 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> There remained several binaries versioned under MSVC and not versioned under
> MinGW. With those fixed, I committed this work.

Thanks! I saw your series of commits and the additional fixes you have done.
--
Michael

On Mon, Jul 14, 2014 at 03:03:28PM -0400, Noah Misch wrote:
> On Fri, Jul 11, 2014 at 11:14:54PM -0700, Michael Paquier wrote:
> > On Fri, Jul 11, 2014 at 10:34 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > > Please fix the MODULES case, perhaps by linking $(WIN32RES) into each library
> > > implicitly.

That idea was somewhat too broad.

> > Yes, that's exactly what I did in the patch attached by linking
> > WIN32RES and a shared library when the lib is built from a single .o
> > file. The rule in src/makefiles/Makefile.win32 has been updated
> > accordingly. pgxs.mk has been updated as well to remove WIN32RES when
> > clean is invoked for MODULES.
>
> There remained several binaries versioned under MSVC and not versioned under
> MinGW. With those fixed, I committed this work.

PostgreSQL 9.5 pgxs tries to link $(WIN32RES) into any single-file module, not
just in-tree modules. Unless the module happens to ship a win32ver.rc file,
the build fails. MODULE_big modules are unaffected.

I plan to fix this as attached, by linking $(WIN32RES) into single-file
modules only when PGFILEDESC is set in the Makefile. That better aligns the
GNU make build system with the heuristic in the MSVC build system's
Project::AddDirResourceFile(). External modules wishing to embed a DLL icon
and/or version information should provide a win32ver.rc and set PGFILEDESC.
(As in released versions, MODULE_big modules must additionally mention
$(WIN32RES) in OBJS.) Modules not desiring Windows file version information
should leave PGFILEDESC unset. In-tree, installed modules do set PGFILEDESC;
to build one using PGXS, you can first build without PGXS and preserve the
generated win32ver.rc.

One alternative I considered was to link with $(WIN32RES) only when PGXS is
unset. That would omit version information from in-tree, single-file modules
built with PGXS. Unlike my plan, it would not break builds of modules that
set PGFILEDESC without intending to trigger inclusion of version information.
However, a grep of the code in PGXN found no affected module.

Thanks,
nm