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

On Thu, Apr 24, 2014 at 7:54 AM, Michael Paquier
<michael.paquier@gmail.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@hagander.net> wrote:

On Thu, Apr 24, 2014 at 7:54 AM, Michael Paquier <michael.paquier@gmail.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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Thu, Apr 24, 2014 at 9:50 AM, Dave Page <dpage@pgadmin.org> wrote:

On Thu, Apr 24, 2014 at 8:27 AM, Magnus Hagander <magnus@hagander.net>
wrote:

On Thu, Apr 24, 2014 at 7:54 AM, Michael Paquier <

michael.paquier@gmail.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@hagander.net> wrote:

On Thu, Apr 24, 2014 at 9:50 AM, Dave Page <dpage@pgadmin.org> wrote:

On Thu, Apr 24, 2014 at 8:27 AM, Magnus Hagander <magnus@hagander.net>
wrote:

On Thu, Apr 24, 2014 at 7:54 AM, Michael Paquier
<michael.paquier@gmail.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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Thu, Apr 24, 2014 at 10:18 AM, Dave Page <dpage@pgadmin.org> wrote:

On Thu, Apr 24, 2014 at 9:13 AM, Magnus Hagander <magnus@hagander.net>
wrote:

On Thu, Apr 24, 2014 at 9:50 AM, Dave Page <dpage@pgadmin.org> wrote:

On Thu, Apr 24, 2014 at 8:27 AM, Magnus Hagander <magnus@hagander.net>
wrote:

On Thu, Apr 24, 2014 at 7:54 AM, Michael Paquier
<michael.paquier@gmail.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@hagander.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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Thu, Apr 24, 2014 at 6:00 PM, Magnus Hagander <magnus@hagander.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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Thu, Apr 24, 2014 at 11:09 AM, Michael Paquier <michael.paquier@gmail.com

wrote:

On Thu, Apr 24, 2014 at 4:27 PM, Magnus Hagander <magnus@hagander.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@hagander.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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Thu, Apr 24, 2014 at 8:31 PM, Michael Paquier
<michael.paquier@gmail.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.paquier@gmail.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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Fri, Jun 20, 2014 at 9:02 PM, MauMau <maumau307@gmail.com> wrote:

From: "Michael Paquier" <michael.paquier@gmail.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.paquier@gmail.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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Sat, Jun 21, 2014 at 1:44 AM, MauMau <maumau307@gmail.com> wrote:

From: "Michael Paquier" <michael.paquier@gmail.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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

From: "Michael Paquier" <michael.paquier@gmail.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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Sat, Jun 21, 2014 at 11:47 AM, MauMau <maumau307@gmail.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.paquier@gmail.com>

On Sat, Jun 21, 2014 at 11:47 AM, MauMau <maumau307@gmail.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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

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@gmail.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@leadboat.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@gmail.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