Review of VS 2010 support patches

-------- Original Message --------
Subject: Review of VS 2010 support patches
From: Craig Ringer <craig@postnewspapers.com.au>
To: PG Hackers <pgsql-hackers@postgresql.org>, Brar Piening <brar@gmx.de>
Date: 05.07.2011 14:25

I haven't had any reply to my email to Brar, so there are a few
details (like whether x64 builds were tested and how x64 required
libraries were built) I could use, but what I've got done so far seems
fine.

I've replied on-list see:
http://archives.postgresql.org/pgsql-hackers/2011-07/msg00066.php

Seems like i've got fooled by "reply to list" being thunderbird's
default for mailing lists once more. Sorry for that one.

The patch (VS2010v7.patch) seems to mix significant changes with
whitespace fixes etc.

Current version (VS2010v8.patch) which I've submitted on-list about one
month ago has fixed this as per Tom Lane's comment.
See: http://archives.postgresql.org/message-id/4DEDB6EE.9060307@gmx.de

pgflex.pl and pgbison.pl
=====================

pgflex.pl and pgbison.pl are a big improvement over the horrid batch
files, but are perhaps too little a translation. There's no need for
the big if(string) then (otherstring) stuff; it can be done much more
cleanly by storing a simple hash of paths to options and doing a file
extension substitution to generate the output filenames. The hash only
needs to be populated for files that get processed with non-default
options, so for pgflex all you need is:

%LEX_OPTS = { 'src\backend\parser\scan.c' -> '-CF' };

I can send adjusted versions of pgflex.pl and pgbison.pl that

I think the approach Andrew Dunstan chose (parsing the Makefiles) is
even more flexible and future proof. We should probably be using his
versions.
See: http://archives.postgresql.org/pgsql-hackers/2011-07/msg00140.php
and http://archives.postgresql.org/pgsql-hackers/2011-07/msg00185.php

DOCUMENTATION
===============

I didn't notice any documentation updates to reflect the fact that
Visual Studio 2010 is now supported. It'd be a good idea to change
install-windows-full.html (or the source of it, anyway) to mention VS
2010 support.

Yep - a clear leftover. I've never written any SGML but I'll try to come
up with something as soon as as I've got the doc build working on my system.

I'm not sure if I'll be able to get 64-bit copies of all the optional
libraries built, so it may be a more minimal build. It'll include at
least zlib, plperl and plpython 64-bit support, though. Information
from Briar about whether he built for 64-bit and if so how he got his
libraries built would help.

Actually my default builds are 64-bit builds as my PC is Win7 x64 and
I'm using 64-Bit versions for my PostgreSQL work.
As you noted, the availability of 64-bit libraries was the limiting
factor for more extensive testing but I haven't run into any Problems
with my default configuration (nothing but plperl) and some others I've
tried yet.

Regards,

Brar

On 6/07/2011 2:15 AM, Brar Piening wrote:

I've replied on-list see:
http://archives.postgresql.org/pgsql-hackers/2011-07/msg00066.php

Ah, sorry I missed that. I generally can't keep up with -hackers and
have to rely on being cc'd.

The patch (VS2010v7.patch) seems to mix significant changes with
whitespace fixes etc.

Current version (VS2010v8.patch) which I've submitted on-list about one
month ago has fixed this as per Tom Lane's comment.
See: http://archives.postgresql.org/message-id/4DEDB6EE.9060307@gmx.de

That's what threw me, actually. The patch is named
"perltidy_before.patch"; I didn't see a separate VS2010v8.patch or link
to one and was trying to figure out how perltidy_before.patch related to
VS2010v7.patch .

It turns out that VS2010v8.patch is also attached to the same message.
Not that you'd know it from the ... interesting ... way the web ui
presents attachments. Sorry I missed it.

I think the approach Andrew Dunstan chose (parsing the Makefiles) is
even more flexible and future proof. We should probably be using his
versions.
See: http://archives.postgresql.org/pgsql-hackers/2011-07/msg00140.php
and http://archives.postgresql.org/pgsql-hackers/2011-07/msg00185.php

That makes sense. Do you want to integrate those in a v9 revision along
wiht a docs patch?

For the docs, it might be worth being more specific about the visual
studio versions. Instead of:

"PostgreSQL supports the compilers from Visual Studio 2005 and Visual
Studio 2008. When using the Platform SDK only, or when building for
64-bit Windows, only Visual Studio 2008 is supported."

I'd suggest writing:

"PostgreSQL supports compilation the compilers shipped with Visual
Studio 2005, 2008 and 2010 (including Express editions), as well as
standalone Windows SDK releases 6.0 to 7.1. Only 32-bit PostgreSQL
builds are supported with SDK versions prior to 6.1 and Visual Studio
versions prior to 2008."

Additionally, it might be worth expanding on "If you wish to build a
64-bit version, you must use the 64-bit version of the command, and vice
versa".

The free SDKs don't install both 32-bit and 64-bit environment start
menu items; they seem to just pick the local host architecture. My 7.1
SDK only has a start menu launcher for x64. So: Perhaps it's worth
mentioning that the "setenv" command can be used from within a Windows
SDK shell to switch architectures. "setenv /?" produces help. For Visual
Studio, use \VC\vcvarsall.bat in your Visual Studio installation
directory. See:
http://msdn.microsoft.com/en-us/library/x4d2c09s(v=VS.100).aspx

Actually my default builds are 64-bit builds as my PC is Win7 x64 and
I'm using 64-Bit versions for my PostgreSQL work.

Ah, OK. Good to know.

I had no problems doing an x64 build using the Windows SDK version 7.1,
and tests passed fine.

Now I just need to test with Windows SDK 6.0 (if I can even get it to
install on win7 x64; the installer keeps crashing) as that's the SDK
shipped with Visual Studio 2005 SP1 .

--
Craig Ringer

POST Newspapers
276 Onslow Rd, Shenton Park
Ph: 08 9381 3088 Fax: 08 9388 2258
ABN: 50 008 917 717
http://www.postnewspapers.com.au/

-------- Original Message --------
Subject: Re: [HACKERS] Review of VS 2010 support patches
From: Craig Ringer <craig@postnewspapers.com.au>
To: Brar Piening <brar@gmx.de>
Date: 06.07.2011 14:56

It turns out that VS2010v8.patch is also attached to the same message.
Not that you'd know it from the ... interesting ... way the web ui
presents attachments. Sorry I missed it.

Yes I've also noticed that the web ui has somewhat screwed up the two
patches attached to my email.

This seems avoidable as one can see in
http://archives.postgresql.org/pgsql-hackers/2011-07/msg00140.php but I
don't know how.

[...]

That makes sense. Do you want to integrate those in a v9 revision
along wiht a docs patch?

I certainly could. But as those files are Andrew's work which isn't
really related to VS2010 build and could as well be commited seperately
I don't want to take credit for it.
I'll remove my versions from the patch (v9 probably) if those files get
commited.

[...]

For the docs, it might be worth being more specific about the visual
studio versions.

[...]

Thanks for the hints I'll consider then once I'll get started with the docs.

[...]

Now I just need to test with Windows SDK 6.0 (if I can even get it to
install on win7 x64; the installer keeps crashing) as that's the SDK
shipped with Visual Studio 2005 SP1 .

Actually I've also successfully tested an empty (no config.pl) 32-bit
build using Visual Studio 2005 RTM.

Regards,

Brar

On 07/06/2011 04:41 PM, Brar Piening wrote:

I certainly could. But as those files are Andrew's work which isn't
really related to VS2010 build and could as well be commited
seperately I don't want to take credit for it.
I'll remove my versions from the patch (v9 probably) if those files
get commited.

I'm just doing some final testing and preparing to commit the new pgflex
and pgbison.

cheers

andrew

-------- Original Message --------
Subject: Re: [HACKERS] Review of VS 2010 support patches
From: Andrew Dunstan <andrew@dunslane.net>
To: Brar Piening <brar@gmx.de>
Date: 06.07.2011 22:58

I'll remove my versions from the patch (v9 probably) if those files
get commited.

I'm just doing some final testing and preparing to commit the new
pgflex and pgbison.

The attached patch includes documentation changes and excludes my
versions of pgbison.pl and pgflex.pl which have been replaced by
Andrews' versions that are already commited.

As before "perltidy_before.patch" has to be applied first and
"VS2010v9.patch" second.

Regards,

Brar

PS: Just in case the web ui concatenates the two patch files again:
perltidy_before.patch has 518 lines with the last line of code being "my
$status = $? >>8;" and VS2010v9.patch has 1608 lines with the last line
of code being "if exist src\backend\win32ver.rc del /q
src\backend\win32ver.rc"

On 7/07/2011 8:26 AM, Brar Piening wrote:

As before "perltidy_before.patch" has to be applied first and
"VS2010v9.patch" second.

OK, I've gone through builds with way too many versions of the Windows
SDK and have test results to report.

The short version: please commit so I never, ever, ever have to do this
again ;-) . I don't see anything newly broken; the only issues I hit
were in master as well, and are probably related to local configuration
issues and/or the sheer profusion of Windows SDK releases I've burdened
my poor laptop with.

Note that x64 builds reported below are configured for plperl and
plpython only. Other config.pl options are left at 'undef'.

Test results:
=============

VS 2005
-------

- SDK 6.0 (VS 2005) x86: OK, vcregress check passed

- SDK 6.0 (VS 2005) x64: OK, vcregress check passed

VS 2008
-------

- SDK 6.1 (VS 2008) x86: OK, vcregress check passed

- SDK 6.1 (VS 2008) x64: Failed - vcbuild exited with code 255.
(Also fails on unpatched git master x64)
Since I'm getting crash report dialogs from
vcbuild, I'm not inclined to blame Pg for this
issue.

- SDK 6.1 (VS 2008) x64 (only plperl enabled): OK, vcregress passed

VS 2010
-------

- SDK 7.0A (VS 2010) x86: OK, vcregress passed
- SDK 7.0A (VS 2010) x64: [Pending, missing x64 tools]

Latest Windows SDK
------------------

- SDK 7.1 x86: OK, vcregress passed
- SDK 7.1 x64: OK (incl. plpython), vcregress passed

Won't test:
===========

- itanium. Does Pg build for itanium as things stand, anyway? Would
anybody notice or care if it didn't?

Not tested yet, unsure if I'll have time
========================================

- vcregress plcheck, vcregress contrib for each combo

- x64 builds with anything more than plperl and plpython enabled.
Library availability is a bit of an issue, and building all those
libraries for x64 is outside what I can currently commit to, especially
as they all require different build methods and some appear to require
patches/fixes to build at all.

- ossp-uuid . No binaries available, doesn't have an NMakefile or
vs project, and

Frankly, I suggest leaving these tests for the buildfarm to sort out. I
don't see any sign of build process issues; they all build fine, and
it's pretty darn unlikely that build changes would cause them to break
at runtime. Windows buildfarm coverage looks pretty decent these days.

--
Craig Ringer

POST Newspapers
276 Onslow Rd, Shenton Park
Ph: 08 9381 3088 Fax: 08 9388 2258
ABN: 50 008 917 717
http://www.postnewspapers.com.au/

-------- Original Message --------
Subject: Re: [HACKERS] Review of VS 2010 support patches
From: Craig Ringer <craig@postnewspapers.com.au>
To: Brar Piening <brar@gmx.de>
Date: 07.07.2011 16:44

Frankly, I suggest leaving these tests for the buildfarm to sort out.
I don't see any sign of build process issues; they all build fine, and
it's pretty darn unlikely that build changes would cause them to break
at runtime. Windows buildfarm coverage looks pretty decent these days.

As I had no Idea whether the buildfarm is even ready to work with VS
2010 I set out and tested it.

I can happily tell you that I have just now completed my first
successful buildfarm run using the attached build-farm.conf

Regards,

Brar

On Thu, Jul 7, 2011 at 02:26, Brar Piening <brar@gmx.de> wrote:

-------- Original Message  --------
Subject: Re: [HACKERS] Review of VS 2010 support patches
From: Andrew Dunstan <andrew@dunslane.net>
To: Brar Piening <brar@gmx.de>
Date: 06.07.2011 22:58

I'll remove my versions from the patch (v9 probably) if those files get
commited.

I'm just doing some final testing and preparing to commit the new pgflex
and pgbison.

The attached patch includes documentation changes and excludes my versions
of pgbison.pl and pgflex.pl which have been replaced by Andrews' versions
that are already commited.

As before "perltidy_before.patch" has to be applied first and
"VS2010v9.patch" second.

Something is strange here. Did you run perltidy with the exact
parameters documented in the README file? If so, perltidy seems to be
version- or platform- dependent. I ran it, and got a slightly
different patch. It's not big differences, but the simple fact that
perltidy doesn't always generate the same result is annoying.

Can you run it again, and make sure you get the exact same diff? So
that it wasn't accidentally run off the wrong version or something?

I've attached the differences between your perltidy and my perltidy run.

I'm using (perltidy -v): "This is perltidy, v20090616"

(My plan was to just commit a perltidy run to keep that part out of
the patch for easier handling, but I'd like to figure out this
difference first..)

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

-------- Original Message --------
Subject: Re: [HACKERS] Review of VS 2010 support patches
From: Magnus Hagander <magnus@hagander.net>
To: Brar Piening <brar@gmx.de>
Date: 08.07.2011 11:38

Sorry for the late response - I've been on a wedding this weekend.

Something is strange here. Did you run perltidy with the exact
parameters documented in the README file?

Yes - I usually even copy paste it from the README as "perltidy -b -bl
-nsfs -naws -l=100 -ole=unix *.pl *.pm" (pasted once more) is hard to
remember and takes a while to type.

If so, perltidy seems to be
version- or platform- dependent. I ran it, and got a slightly
different patch. It's not big differences, but the simple fact that
perltidy doesn't always generate the same result is annoying.

Can you run it again, and make sure you get the exact same diff? So
that it wasn't accidentally run off the wrong version or something?

I just rechecked that applying my two patches vs. applying my two
patches + running the above perltidy command gives no difference (0 byte
patch).

I've attached the differences between your perltidy and my perltidy run.

I'm using (perltidy -v): "This is perltidy, v20090616"

I'm currently using (perl -v): "This is perl 5, version 14, subversion 1
(v5.14.1) built for MSWin32-x64-multi-thread"
and
(perltidy -v): "This is perltidy, v20101217"

But I've just recently upgraded to the latest Perl version.
The patch has been produced using some 5.12.? ActivePerl and it's
corresponding perltidy version which (whatever it was) obviously
produced the same result for me.

http://perltidy.sourceforge.net/ChangeLog.html#2010_12_17 doesn't seem
to have any Information which would explain our different patches.

Strange...

Regards,

Brar

On Sun, Jul 10, 2011 at 20:46, Brar Piening <brar@gmx.de> wrote:

Sorry for the late response - I've been on a wedding this weekend.

Something is strange here. Did you run perltidy with the exact
parameters documented in the README file?

Yes - I usually even copy paste it from the README as "perltidy -b -bl -nsfs
-naws -l=100 -ole=unix *.pl *.pm" (pasted once more) is hard to remember and
takes a while to type.

Bleh, that's annoying - that means it behaves different in different versions :S

If so, perltidy seems to be
version- or platform- dependent. I ran it, and got a slightly
different patch. It's not big differences, but the simple fact that
perltidy doesn't always generate the same result is annoying.

Can you run it again, and make sure you get the exact same diff? So
that it wasn't accidentally run off the wrong version or something?

I just rechecked that applying my two patches vs. applying my two patches +
running the above perltidy command gives no difference (0 byte patch).

I've attached the differences between your perltidy and my perltidy run.

I'm using (perltidy -v): "This is perltidy, v20090616"

I'm currently using (perl -v): "This is perl 5, version 14, subversion 1
(v5.14.1) built for MSWin32-x64-multi-thread"
and
(perltidy -v): "This is perltidy, v20101217"

But I've just recently upgraded to the latest Perl version.
The patch has been produced using some 5.12.? ActivePerl and it's
corresponding perltidy version which (whatever it was) obviously produced
the same result for me.

I'm using 5.10... Not sure if it's the perl version or more likely the
perltidy version that causes the difference, but there's not too much
we can do about that. I'm not sure the differences are big enough that
we actually want to care about it - I think it's easier to just take
changes caused by it out of each commit. We're still getting the large
majority as the same.

So - for now, I have made a perltidy run and committed it, which
should make it slightly easier for reviewing the actual patch :-)

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

On 07/06/2011 08:26 PM, Brar Piening wrote:

-------- Original Message --------
Subject: Re: [HACKERS] Review of VS 2010 support patches
From: Andrew Dunstan <andrew@dunslane.net>
To: Brar Piening <brar@gmx.de>
Date: 06.07.2011 22:58

I'll remove my versions from the patch (v9 probably) if those files
get commited.

I'm just doing some final testing and preparing to commit the new
pgflex and pgbison.

The attached patch includes documentation changes and excludes my
versions of pgbison.pl and pgflex.pl which have been replaced by
Andrews' versions that are already commited.

As before "perltidy_before.patch" has to be applied first and
"VS2010v9.patch" second.

I just started looking at this a bit. One small question: why are we
using "use base qw(foo);" instead of "use parent qw(foo);" which I
understand is preferred these days?

cheers

andrew

On Sun, Jul 31, 2011 at 03:25, Andrew Dunstan <andrew@dunslane.net> wrote:

On 07/06/2011 08:26 PM, Brar Piening wrote:

-------- Original Message  --------
Subject: Re: [HACKERS] Review of VS 2010 support patches
From: Andrew Dunstan <andrew@dunslane.net>
To: Brar Piening <brar@gmx.de>
Date: 06.07.2011 22:58

I'll remove my versions from the patch (v9 probably) if those files get
commited.

I'm just doing some final testing and preparing to commit the new pgflex
and pgbison.

The attached patch includes documentation changes and excludes my versions
of pgbison.pl and pgflex.pl which have been replaced by Andrews' versions
that are already commited.

As before "perltidy_before.patch" has to be applied first and
"VS2010v9.patch" second.

I just started looking at this a bit. One small question: why are we using
"use base qw(foo);" instead of "use parent qw(foo);" which I understand is
preferred these days?

I am no perl expert, but I see we are using this already today - in
code written by you in one case ;) I'd assume it was just following
the same standard... If the other way is the way to do it today, I see
no reason not to change it to use that.

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

On Wed, Aug 10, 2011 at 9:03 AM, Magnus Hagander <magnus@hagander.net> wrote:

I am no perl expert, but I see we are using this already today - in
code written by you in one case ;) I'd assume it was just following
the same standard... If the other way is the way to do it today, I see
no reason not to change it to use that.

This is the first I'm hearing of use parent - has that been around
long enough that we needn't worry about breaking old Perl versions?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 08/10/2011 09:03 AM, Magnus Hagander wrote:

On Sun, Jul 31, 2011 at 03:25, Andrew Dunstan<andrew@dunslane.net> wrote:

On 07/06/2011 08:26 PM, Brar Piening wrote:

-------- Original Message --------
Subject: Re: [HACKERS] Review of VS 2010 support patches
From: Andrew Dunstan<andrew@dunslane.net>
To: Brar Piening<brar@gmx.de>
Date: 06.07.2011 22:58

I'll remove my versions from the patch (v9 probably) if those files get
commited.

I'm just doing some final testing and preparing to commit the new pgflex
and pgbison.

The attached patch includes documentation changes and excludes my versions
of pgbison.pl and pgflex.pl which have been replaced by Andrews' versions
that are already commited.

As before "perltidy_before.patch" has to be applied first and
"VS2010v9.patch" second.

I just started looking at this a bit. One small question: why are we using
"use base qw(foo);" instead of "use parent qw(foo);" which I understand is
preferred these days?

I am no perl expert, but I see we are using this already today - in
code written by you in one case ;) I'd assume it was just following
the same standard... If the other way is the way to do it today, I see
no reason not to change it to use that.

Umm, where are we using it today?

[andrew@emma pg_head]$ grep -r -P 'use\s+base' .
./doc/src/sgml/release-old.sgml: what lexer you use based on the
platform you use.
./doc/src/sgml/charset.sgml: encoding to use based on the
specified or default locale.
./src/backend/commands/aggregatecmds.c: * Old style: use
basetype parameter. This supports aggregates of
./autom4te.cache/output.0:# Required to use basename.
./autom4te.cache/output.0:# Required to use basename.
./configure:# Required to use basename.
./configure:# Required to use basename.
[andrew@emma pg_head]$

cheers

andrew

On 08/10/2011 09:21 AM, Robert Haas wrote:

On Wed, Aug 10, 2011 at 9:03 AM, Magnus Hagander<magnus@hagander.net> wrote:

I am no perl expert, but I see we are using this already today - in
code written by you in one case ;) I'd assume it was just following
the same standard... If the other way is the way to do it today, I see
no reason not to change it to use that.

This is the first I'm hearing of use parent - has that been around
long enough that we needn't worry about breaking old Perl versions?

Good question. Maybe not.

cheers

andrew

On Wed, Aug 10, 2011 at 15:25, Andrew Dunstan <andrew@dunslane.net> wrote:

On 08/10/2011 09:03 AM, Magnus Hagander wrote:

On Sun, Jul 31, 2011 at 03:25, Andrew Dunstan<andrew@dunslane.net>  wrote:

On 07/06/2011 08:26 PM, Brar Piening wrote:

-------- Original Message  --------
Subject: Re: [HACKERS] Review of VS 2010 support patches
From: Andrew Dunstan<andrew@dunslane.net>
To: Brar Piening<brar@gmx.de>
Date: 06.07.2011 22:58

I'll remove my versions from the patch (v9 probably) if those files
get
commited.

I'm just doing some final testing and preparing to commit the new
pgflex
and pgbison.

The attached patch includes documentation changes and excludes my
versions
of pgbison.pl and pgflex.pl which have been replaced by Andrews'
versions
that are already commited.

As before "perltidy_before.patch" has to be applied first and
"VS2010v9.patch" second.

I just started looking at this a bit. One small question: why are we
using
"use base qw(foo);" instead of "use parent qw(foo);" which I understand
is
preferred these days?

I am no perl expert, but I see we are using this already today - in
code written by you in one case ;) I'd assume it was just following
the same standard... If the other way is the way to do it today, I see
no reason not to change it to use that.

Umm, where are we using it today?

  [andrew@emma pg_head]$ grep -r -P 'use\s+base' .
  ./doc/src/sgml/release-old.sgml:   what lexer you use based on the
  platform you use.
  ./doc/src/sgml/charset.sgml:     encoding to use based on the
  specified or default locale.
  ./src/backend/commands/aggregatecmds.c:         * Old style: use
  basetype parameter.  This supports aggregates of
  ./autom4te.cache/output.0:# Required to use basename.
  ./autom4te.cache/output.0:# Required to use basename.
  ./configure:# Required to use basename.
  ./configure:# Required to use basename.
  [andrew@emma pg_head]$

Meh. I am clearly not back in the game since my vacation. I didn't
realize base was a keyword... Ignore and move on, nothing to see here.

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

Brar Piening wrote:

The attached patch includes documentation changes and excludes my
versions of pgbison.pl and pgflex.pl which have been replaced by
Andrews' versions that are already commited.

Building current head today I noticed that the patch doesn't apply
cleanly anymore.

Attached is a new version.

Regards,

Brar

On 09/28/2011 03:53 PM, Brar Piening wrote:

Brar Piening wrote:

The attached patch includes documentation changes and excludes my
versions of pgbison.pl and pgflex.pl which have been replaced by
Andrews' versions that are already commited.

Building current head today I noticed that the patch doesn't apply
cleanly anymore.

Attached is a new version.

This patch looks OK in principle.

Some minor nitpicks:

Do we really need to create all those VSnnnnProject.pm and
VSnnnnSolution.pm files? They are all always included anyway. Why not
just stash all the packages in Solution.pm and Project.pm? Also, instead
of doing this in Mkvcbuild.pm:

my $vsVersion = VSObjectFactory::DetermineVisualStudioVersion();
$solution = VSObjectFactory::CreateSolution($vsVersion, $config);

why not just add "use VSObjectFactory;" at the top of the file and
import these into the current namespace, just as we do for pretty much
everything else?

There are some stylistic things that aren't the way I usually do things
(use of named instead of anonymous file handles, use of heredocs instead
of qq{} style quotes) and that I would prefer done differently, but
those are more matters of taste than substance. I also generally dislike
composing XML by non-formal means, as it can be quite error prone and
often leads to errors in unforeseen corner cases. But in this case we
certainly don't want to impose an extra requirement on some perl XML
module, and it would make this code terribly verbose, so we just have to
hope we get it right :-)

I don't have a VS2010 machine available to test it on unfortunately.
I'll see what I can do about arranging one, at least temporarily.
Meanwhile I'll test it on my VS2005 and VS2008 machines to make sure it
doesn't break anything.

cheers

andrew

Andrew Dunstan wrote:

Some minor nitpicks:

Do we really need to create all those VSnnnnProject.pm and
VSnnnnSolution.pm files? They are all always included anyway. Why not
just stash all the packages in Solution.pm and Project.pm?

We certainly don't *need* them.
Having different files separates the tasks of generating different
target file formats into different source files. In my opinion this
makes it easier to find the code that is actually generating the files
that get used in a specific build environment.
While the VSnnnnSolution.pm and VC200nProject.pm files are indeed not
much more than stubs that could eventually be extended in future (and
probably never will) VC2010Project.pm contains the whole code for
generating the new file format which would significantly bloat up the
code in Project.pm that currently contains the common code for
generating the old file formats.

Anyhow - this is just my opinion and my intention is to help improving
the Windows build process and not forcing my design into the project.

Also, instead of doing this in Mkvcbuild.pm:

my $vsVersion = VSObjectFactory::DetermineVisualStudioVersion();
$solution = VSObjectFactory::CreateSolution($vsVersion, $config);

why not just add "use VSObjectFactory;" at the top of the file and
import these into the current namespace, just as we do for pretty much
everything else?

Yes - my way (singleton, clean namespace) is probably overengineering in
this context.

There are some stylistic things that aren't the way I usually do
things (use of named instead of anonymous file handles, use of
heredocs instead of qq{} style quotes) and that I would prefer done
differently, but those are more matters of taste than substance.

Please go ahead and change it to whatever style you prefer. There is
certainly more than one way to style it ;-)

I also generally dislike composing XML by non-formal means, as it can
be quite error prone and often leads to errors in unforeseen corner
cases. But in this case we certainly don't want to impose an extra
requirement on some perl XML module, and it would make this code
terribly verbose, so we just have to hope we get it right :-)

I actually had a look into the default ActivePerl docs to find out
whether there is a better way for generating xml, but as there is no
XML-generator package in the default distribution I decided not to
introduce a new dependency.

Thanks for your feedback.

Regards,

Brar