Date: Mon, 1 Oct 2012 21:12:01 -0400
From: Noah Misch <noah@leadboat.com>
To: Brar Piening <brar@gmx.de>
Cc: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>,
	Magnus Hagander <magnus@hagander.net>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Visual Studio 2012 RC
Message-ID: <20121002011201.GC30396@tornado.leadboat.com>
References: <4FCA84F6.5090706@gmx.de>
	<CABUevEzT4B+MyM6-VH4_vvg1KFys4XAAKQcaKbbHRKH7mF7SSw@mail.gmail.com>
	<4FD3F553.9060105@gmx.de>
	<CABUevEwEYa8Ay5aK9kk+FPCWs6xiwnW-e1B2mcx3Ff3NMaBuhg@mail.gmail.com>
	<4FEB0D39.6080108@enterprisedb.com> <50526BA6.3030103@gmx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <50526BA6.3030103@gmx.de>
User-Agent: Mutt/1.5.18 (2008-05-17)

On Fri, Sep 14, 2012 at 01:26:30AM +0200, Brar Piening wrote:
> Heikki Linnakangas wrote:
>> I don't think we can realistically support VS2012 until Microsoft  
>> releases the gratis Visual Studio Express 2012 for Windows Desktop.
>
> As they've released now I've updated my Patch with docs and ask for review.

I used this patch to build 64-bit PostgreSQL under Windows 7 Professional,
using Visual Studio 2012 Express for Windows Desktop.  I did not provide a
config.pl, so this build used the defaults -- in particular, it did not
exercise linking to optional external projects like perl and libxml2.  A
"vcregress check" passed.  The build emitted no warnings beyond those seen on
buildfarm member "chough" (VS 2008 Express).

My build log filled 8.8 MiB, a large increase from the 432 KiB of the "chough"
build log.  This isn't strictly a problem, but do you happen to have ideas for
curbing the noise?

I find no functional problems with the patch, but some comment updates and
other trivia need attention.  The patch itself was reversed; it applied
cleanly with "patch -R".  I regenerated it in the usual direction for the
portions I quote below.

  src/tools/msvc/MSBuildProject.pm:4:# Package that encapsulates a MSBuild (Visual C++ 2010) project file

Say something like "Visual C++ 2010 or greater".

  src/tools/msvc/VSObjectFactory.pm:93:# we use nmake as it has existed for a long time and still exists in visual studio 2010

Likewise, modify this comnent to be open-ended.  If nmake ever disappears,
we'll be updating this code and can see to change the comment.

> *** a/doc/src/sgml/install-windows.sgml
> --- b/doc/src/sgml/install-windows.sgml
> ***************
> *** 22,28 ****
>     Microsoft tools is to install a supported version of the
>     <productname>Microsoft Windows SDK</productname> and use the included
>     compiler. It is also possible to build with the full
> !   <productname>Microsoft Visual C++ 2005, 2008 or 2010</productname>. In some cases
>     that requires the installation of the <productname>Windows SDK</productname>
>     in addition to the compiler.
>    </para>
> --- 22,28 ----
>     Microsoft tools is to install a supported version of the
>     <productname>Microsoft Windows SDK</productname> and use the included
>     compiler. It is also possible to build with the full
> !   <productname>Microsoft Visual C++ 2005, 2008, 2010 or 2012</productname>. In some cases
>     that requires the installation of the <productname>Windows SDK</productname>
>     in addition to the compiler.
>    </para>

I think this paragraph should be more like the one in the next patch hunk:
call out Visual Studio 2012 Express for Windows Desktop and Windows SDK 7.1 as
the main recommendations.  Perhaps even demote the SDK entirely and just
recommend VS 2012.  It'd odd to recommend only a past-version tool if a
current-version tool works just as well.

> ***************
> *** 77,90 ****
>     <productname>Visual Studio Express</productname> or some versions of the
>     <productname>Microsoft Windows SDK</productname>. If you do not already have a
>     <productname>Visual Studio</productname> environment set up, the easiest
> !   way is to use the compilers in the <productname>Windows SDK</productname>,
> !   which is a free download from Microsoft.
>    </para>
>   
>    <para>
>     PostgreSQL is known to support compilation using the compilers shipped with
>     <productname>Visual Studio 2005</productname> to
> !   <productname>Visual Studio 2010</productname> (including Express editions),
>     as well as standalone Windows SDK releases 6.0 to 7.1.
>     64-bit PostgreSQL builds are only supported with
>     <productname>Microsoft Windows SDK</productname> version 6.0a and above or
> --- 77,91 ----
>     <productname>Visual Studio Express</productname> or some versions of the
>     <productname>Microsoft Windows SDK</productname>. If you do not already have a
>     <productname>Visual Studio</productname> environment set up, the easiest
> !   ways are to use the compilers in the <productname>Windows SDK</productname>

I would write "Windows SDK 7.1" here and remove the parenthesized bit.
There's a later mention of support for older versions.

> !   (<= 7.1) or those from <productname>Visual Studio Express 2012 for Windows
> !   Desktop</productname>, which are both free downloads from Microsoft.

>    </para>
>   
>    <para>
>     PostgreSQL is known to support compilation using the compilers shipped with
>     <productname>Visual Studio 2005</productname> to
> !   <productname>Visual Studio 2012</productname> (including Express editions),
>     as well as standalone Windows SDK releases 6.0 to 7.1.
>     64-bit PostgreSQL builds are only supported with
>     <productname>Microsoft Windows SDK</productname> version 6.0a and above or
> ***************
> *** 157,162 **** $ENV{PATH}=$ENV{PATH} . ';c:\some\where\bison\bin';
> --- 158,165 ----
>         If you install the <productname>Windows SDK</productname>
>         including the <application>Visual C++ Compilers</application>,
>         you don't need <productname>Visual Studio</productname> to build.
> +       Note that as of Version 8.0a the Windows SDK no longer ships with a
> +       complete command-line build environment.

The part ending here looks like this:

    <varlistentry>
     <term><productname>Microsoft Windows SDK</productname></term>
     <listitem><para>
      It is recommended that you upgrade to the latest supported version
      of the <productname>Microsoft Windows SDK</productname> (currently
      version 7.1), available for download from
      <ulink url="http://www.microsoft.com/downloads/"></>.
     </para>
     <para>
      You must always include the
      <application>Windows Headers and Libraries</application> part of the SDK.
      If you install the <productname>Windows SDK</productname>
      including the <application>Visual C++ Compilers</application>,
      you don't need <productname>Visual Studio</productname> to build.
      Note that as of Version 8.0a the Windows SDK no longer ships with a
      complete command-line build environment.
     </para></listitem>
    </varlistentry>

Since SDK version 7.1 is named as the "latest supported version", I understand
from that text that installing SDK version 8.0a along with compilers from
another source (VS 2012 full, VS 2012 Express for Desktop) is considered
"unsupported" as a PostgreSQL build environment.  Is that your intent?

> *** a/src/tools/msvc/MSBuildProject.pm
> --- b/src/tools/msvc/MSBuildProject.pm
> ***************
> *** 397,400 **** sub new
> --- 397,440 ----
>   	return $self;
>   }
>   
> + package VC2012Project;
> + 
> + #
> + # Package that encapsulates a Visual C++ 2012 project file
> + #
> + 
> + use strict;
> + use warnings;
> + use base qw(MSBuildProject);
> + 
> + sub new
> + {
> +     my $classname = shift;
> +     my $self = $classname->SUPER::_new(@_);
> +     bless($self, $classname);
> + 
> +     $self->{vcver} = '11.00';
> + 
> +     return $self;
> + }
> + 
> + sub WriteConfigurationPropertyGroup

Please add a comment explaining what this override does differently.  (I think
it just adds the "<PlatformToolset>" element.)

> + {
> +     my ($self, $f, $cfgname, $p) = @_;
> +     my $cfgtype =
> +       ($self->{type} eq "exe")
> +       ?'Application'
> +       :($self->{type} eq "dll"?'DynamicLibrary':'StaticLibrary');
> + 
> +     print $f <<EOF;
> +   <PropertyGroup Condition="'\$(Configuration)|\$(Platform)'=='$cfgname|$self->{platform}'" Label="Configuration">
> +     <ConfigurationType>$cfgtype</ConfigurationType>
> +     <UseOfMfc>false</UseOfMfc>
> +     <CharacterSet>MultiByte</CharacterSet>
> +     <WholeProgramOptimization>$p->{wholeopt}</WholeProgramOptimization>
> +     <PlatformToolset>v110</PlatformToolset>
> +   </PropertyGroup>
> + EOF
> + }
> + 
>   1;

I'm marking this patch Waiting on Author, but the changes needed to get it
Ready for Committer are fairly trivial.

Thanks,
nm