Using Windows SDK v7.1 when building in MSBuild (Visual Studio 2010/2010e).

The 'msbuild_toolset%' variable can be used to select the desired version of Windows SDK when building in MSBuild (Visual Studio 2010/2010e).

TEST=Built all.sln successfully.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124274

[alexeypa (please no reviews), 2012-02-24 23:42:59]

Hi,

This change makes Gyp emit projects that properly refer to Windows 7.1 SDK
(instead of hacking system files). Gyp supports the 'msbuild_toolset' property
as of http://gyp.googlecode.com/svn/trunk@1230. 

Here is some more background:
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

Please, take a look (or suggest a more appropriate reviewer).

Thanks.

[Nico, 2012-02-27 15:57:32]

maruel or scottmg are more windowsy than I am, so they should probably review
this.

I believe on my windows laptop, msvc2010 came with the 7.0 sdk. But the chrome
build instructions do say "install 7.1sdk" (which I ignored :-) ), so I guess
this is fine.

[M-A Ruel, 2012-02-27 16:30:41]

I'll defer to Scott.

[alexeypa (please no reviews), 2012-02-27 16:41:45]

On 2012/02/27 15:57:32, Nico wrote:
> I believe on my windows laptop, msvc2010 came with the 7.0 sdk. But the chrome
> build instructions do say "install 7.1sdk" (which I ignored :-) ), so I guess
> this is fine.

This is a good point. I assumed that SDK 7.1 is a hard requirement but:

1. This might be not the case.
2. People do not build the whole tree, so other versions of SDK could be used. 

So I uploaded a new version of the CL. Rather than forcing SDK 7.1, it uses the
default version of SDK (i.e. it gives the same behavior as the current
configuration). But setting 'msbuild_toolset' ~/.gyp/include.gypi makes Gyp to
pick the desired version of SDK. (for instance {'variables':
{'msbuild_toolset'='Windows7.1SDK'}} ).

Once thic CL is in the tree I'll be able to update the Building on Windows page,
instructing folks to update their's common.gypi.


It does not set msbuild_toolset by default, meaning that nothing would change in
anyone's configuration.

[alexeypa (please no reviews), 2012-02-27 16:42:42]

On 2012/02/27 16:41:45, alexeypa wrote:
> It does not set msbuild_toolset by default, meaning that nothing would change
in
> anyone's configuration.

While true, this is just a leftover from my editing of the previous comment. :-)

[Nico, 2012-02-27 18:09:06]

Looks good to me with comment tweak.

http://codereview.chromium.org/9447062/diff/5002/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/9447062/diff/5002/build/common.gypi#newcode846
build/common.gypi:846: # default) should be specified in common.gypi.
s/common.gypi/include.gypi/, right?

[scottmg, 2012-02-27 19:00:37]

If it's possible now, I think we should explicitly set an SDK that we know
works as the default, rather than leaving it up to whims of what's
configured on the machine. Allowing an override in include.gypi is fine if
people want to test newer versions of course.

I don't know if The Version should be 7.0A or 7.1 though. Can you confirm
if we can build successfully with 7.0A or if there's already code that
requires 7.1?

(I know your change isn't making anything "worse", but since you seem to
know the most about this problem, it'd be great if you could look into it)

On Mon, Feb 27, 2012 at 10:09 AM, <thakis@chromium.org> wrote:

> Looks good to me with comment tweak.
>
>
>
http://codereview.chromium.**org/9447062/diff/5002/build/**common.gypi<http:/...
> File build/common.gypi (right):
>
> http://codereview.chromium.**org/9447062/diff/5002/build/**
>
common.gypi#newcode846<http://codereview.chromium.org/9447062/diff/5002/build/common.gypi#newcode846>
> build/common.gypi:846: # default) should be specified in common.gypi.
> s/common.gypi/include.gypi/, right?
>
>
http://codereview.chromium.**org/9447062/<http://codereview.chromium.org/9447...
>

[alexeypa (please no reviews), 2012-02-27 19:51:44]

On 2012/02/27 19:00:37, scottmg wrote:
> I don't know if The Version should be 7.0A or 7.1 though. Can you confirm
> if we can build successfully with 7.0A or if there's already code that
> requires 7.1?
> 
> (I know your change isn't making anything "worse", but since you seem to
> know the most about this problem, it'd be great if you could look into it)

I'll try building all.sln with 7.0A and see what happens. 

In the meanwhile, looking at the building instruction page history the SDK
version was bumped from 7.0 to 7.1 by maruel (on March 29, 2011). Do you happen
to remember why was it changed?

[M-A Ruel, 2012-02-27 19:54:10]

On 2012/02/27 19:51:44, alexeypa wrote:
> On 2012/02/27 19:00:37, scottmg wrote:
> > I don't know if The Version should be 7.0A or 7.1 though. Can you confirm
> > if we can build successfully with 7.0A or if there's already code that
> > requires 7.1?
> > 
> > (I know your change isn't making anything "worse", but since you seem to
> > know the most about this problem, it'd be great if you could look into it)
> 
> I'll try building all.sln with 7.0A and see what happens. 
> 
> In the meanwhile, looking at the building instruction page history the SDK
> version was bumped from 7.0 to 7.1 by maruel (on March 29, 2011). Do you
happen
> to remember why was it changed?

I think it was Ben that had requested this for some Windows7 functions or macros
not visible in the 7.0a SDK.

[alexeypa (please no reviews), 2012-02-28 21:02:59]

On 2012/02/27 19:51:44, alexeypa wrote:
> On 2012/02/27 19:00:37, scottmg wrote:
> > I don't know if The Version should be 7.0A or 7.1 though. Can you confirm
> > if we can build successfully with 7.0A or if there's already code that
> > requires 7.1?
> > 
> > (I know your change isn't making anything "worse", but since you seem to
> > know the most about this problem, it'd be great if you could look into it)
> 
> I'll try building all.sln with 7.0A and see what happens. 

Actually I was able to build all.sln with 7.0A SDK just fine. (I've got a few
"sharing violation" errors apparently caused by parallel build issues MSBuild
still suffers from). Given that the default value of the msbuild_toolset
variable should be empty. This will cause MSBuild to pick the default SDK
version for the compiler used. (Which is also the current behavior.) I don't
think we need to fix it to "v100" because it will break if someone tries the
next version of Visual Studio. SDK 7.0/7.1 users can be instructed to override
msbuild_tools in ~/.gyp/include.gypi.

[alexeypa (please no reviews), 2012-02-28 21:03:08]

PTAL

https://chromiumcodereview.appspot.com/9447062/diff/5002/build/common.gypi
File build/common.gypi (right):

https://chromiumcodereview.appspot.com/9447062/diff/5002/build/common.gypi#ne...
build/common.gypi:846: # default) should be specified in common.gypi.
On 2012/02/27 18:09:07, Nico wrote:
> s/common.gypi/include.gypi/, right?

Yes, indeed.

[scottmg, 2012-02-28 21:09:15]

On 2012/02/28 21:02:59, alexeypa wrote:
> On 2012/02/27 19:51:44, alexeypa wrote:
> > On 2012/02/27 19:00:37, scottmg wrote:
> > > I don't know if The Version should be 7.0A or 7.1 though. Can you confirm
> > > if we can build successfully with 7.0A or if there's already code that
> > > requires 7.1?
> > > 
> > > (I know your change isn't making anything "worse", but since you seem to
> > > know the most about this problem, it'd be great if you could look into it)
> > 
> > I'll try building all.sln with 7.0A and see what happens. 
> 
> Actually I was able to build all.sln with 7.0A SDK just fine. (I've got a few
> "sharing violation" errors apparently caused by parallel build issues MSBuild
> still suffers from). Given that the default value of the msbuild_toolset
> variable should be empty. This will cause MSBuild to pick the default SDK
> version for the compiler used. (Which is also the current behavior.) I don't
> think we need to fix it to "v100" because it will break if someone tries the
> next version of Visual Studio. SDK 7.0/7.1 users can be instructed to override
> msbuild_tools in ~/.gyp/include.gypi.

Thanks for check into this Alex. lgtm.

[commit-bot: I haz the power, 2012-02-29 17:24:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/9447062/18001

[commit-bot: I haz the power, 2012-02-29 20:39:28]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2012-02-29 20:40:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/9447062/18001

[commit-bot: I haz the power, 2012-02-29 22:53:31]

Change committed as 124274