Update Windows version macros to Win 7

Update Windows version macros to Win 7

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

[jschuh, 2011-09-23 22:44:05]

Adding you in now, but lets wait to see how the trybot run goes.

[cpu_(ooo_6.6-7.5), 2011-09-23 22:50:47]

lgtm

[cpu_(ooo_6.6-7.5), 2011-09-23 22:51:48]

I am confused now, the last patchset does not have angle..

[jschuh, 2011-09-23 23:01:33]

On 2011/09/23 22:51:48, cpu wrote:
> I am confused now, the last patchset does not have angle..

Yes... but now you are enlightened. ;)

[jschuh, 2011-09-23 23:38:42]

@brettw - I needed to move a base macro from a .cc to a header.

[M-A Ruel, 2011-09-24 03:14:54]

Looks like it breaks incremental compile.

http://build.chromium.org/p/tryserver.chromium/builders/win_sync/builds/5545/...

with errors like:

e:\b\build\slave\win\build\src\base\command_line.cc : warning C4651:
'/D_WIN32_WINNT=0x0601' specified for precompiled header but not for current
compile

But how come precompiled headers are used? Joi, we should disable that, that's a
recipe for pain and suffering in infrastructure.

[Jói, 2011-09-24 19:06:12]

> But how come precompiled headers are used? Joi, we should disable that,
> that's a
> recipe for pain and suffering in infrastructure.

I'd like to understand more about the pain, so that we can make the
right tradeoff.  Using preceompiled headers currently speeds up builds
by about 18% from what they were before (for the 'chrome' target,
maybe more when building all targets) and are likely to speed up the
build by a total of about 25% once I make a similar change to WebKit
to use precompiled headers (when building the same target).  So the
upside for keeping it on is considerable for developers and for bot
cycle times.

Are there situations other than the current one that make this painful
for infrastructure?  I think the current issue shouldn't come up more
than a handful of times per year?

Cheers,
Jói


On Sat, Sep 24, 2011 at 3:14 AM,  <maruel@chromium.org> wrote:
> Looks like it breaks incremental compile.
>
>
http://build.chromium.org/p/tryserver.chromium/builders/win_sync/builds/5545/...
>
> with errors like:
>
> e:\b\build\slave\win\build\src\base\command_line.cc : warning C4651:
> '/D_WIN32_WINNT=0x0601' specified for precompiled header but not for current
> compile
>
> But how come precompiled headers are used? Joi, we should disable that,
> that's a
> recipe for pain and suffering in infrastructure.
>
> http://codereview.chromium.org/8004004/
>

[Jói, 2011-09-24 20:08:14]

BTW I already turned off use of precompiled headers in release builds to
avoid OOM problems in Canary builds.

(sent from phone, pardon brevity)
On Sep 24, 2011 7:05 PM, "Jói Sigurðsson" <joi@chromium.org> wrote:
>> But how come precompiled headers are used? Joi, we should disable that,
>> that's a
>> recipe for pain and suffering in infrastructure.
>
> I'd like to understand more about the pain, so that we can make the
> right tradeoff. Using preceompiled headers currently speeds up builds
> by about 18% from what they were before (for the 'chrome' target,
> maybe more when building all targets) and are likely to speed up the
> build by a total of about 25% once I make a similar change to WebKit
> to use precompiled headers (when building the same target). So the
> upside for keeping it on is considerable for developers and for bot
> cycle times.
>
> Are there situations other than the current one that make this painful
> for infrastructure? I think the current issue shouldn't come up more
> than a handful of times per year?
>
> Cheers,
> Jói
>
>
> On Sat, Sep 24, 2011 at 3:14 AM, <maruel@chromium.org> wrote:
>> Looks like it breaks incremental compile.
>>
>>
http://build.chromium.org/p/tryserver.chromium/builders/win_sync/builds/5545/...
>>
>> with errors like:
>>
>> e:\b\build\slave\win\build\src\base\command_line.cc : warning C4651:
>> '/D_WIN32_WINNT=0x0601' specified for precompiled header but not for
current
>> compile
>>
>> But how come precompiled headers are used? Joi, we should disable that,
>> that's a
>> recipe for pain and suffering in infrastructure.
>>
>> http://codereview.chromium.org/8004004/
>>

[M-A Ruel, 2011-09-24 20:29:34]

The main problem is that MSVC PCH handler doesn't handle changes in command
line defines safely. Like I pasted, it generates a warning C4651, treated as
error, instead of doing the right thing and regenerating the PCH when
project level defines change.

M-A

Le 24 septembre 2011 16:08, Jói Sigurðsson <joi@chromium.org> a écrit :

> BTW I already turned off use of precompiled headers in release builds to
> avoid OOM problems in Canary builds.
>
> (sent from phone, pardon brevity)
> On Sep 24, 2011 7:05 PM, "Jói Sigurðsson" <joi@chromium.org> wrote:
> >> But how come precompiled headers are used? Joi, we should disable that,
> >> that's a
> >> recipe for pain and suffering in infrastructure.
> >
> > I'd like to understand more about the pain, so that we can make the
> > right tradeoff. Using preceompiled headers currently speeds up builds
> > by about 18% from what they were before (for the 'chrome' target,
> > maybe more when building all targets) and are likely to speed up the
> > build by a total of about 25% once I make a similar change to WebKit
> > to use precompiled headers (when building the same target). So the
> > upside for keeping it on is considerable for developers and for bot
> > cycle times.
> >
> > Are there situations other than the current one that make this painful
> > for infrastructure? I think the current issue shouldn't come up more
> > than a handful of times per year?
> >
> > Cheers,
> > Jói
> >
> >
> > On Sat, Sep 24, 2011 at 3:14 AM, <maruel@chromium.org> wrote:
> >> Looks like it breaks incremental compile.
> >>
> >>
>
http://build.chromium.org/p/tryserver.chromium/builders/win_sync/builds/5545/...
> >>
> >> with errors like:
> >>
> >> e:\b\build\slave\win\build\src\base\command_line.cc : warning C4651:
> >> '/D_WIN32_WINNT=0x0601' specified for precompiled header but not for
> current
> >> compile
> >>
> >> But how come precompiled headers are used? Joi, we should disable that,
> >> that's a
> >> recipe for pain and suffering in infrastructure.
> >>
> >> http://codereview.chromium.org/8004004/
> >>
>

[Jói, 2011-09-24 21:10:13]

OK, so we need to consider how many times those kinds of changes
occur, and decide if the cost of handling them is greater or lesser
than the gain of ~25% faster builds for developers and cycle times for
Windows compiles on the bots.

So I guess we need answers to the following questions:
a) How many times do you think we change command-line flags per year,
and what is the cost of handling the problem that this plus
precompiled headers causes?
b) Also, are there any workarounds?
c) Does VS 2010 perhaps fix the problem (it seems it would be correct
for VS to rebuild the PCH if project-level defines are changed)?

Note that the use of precompiled headers could be made optional, used
mostly by developers and not tested on the waterfall like the
component build initially was, but I think we should give it some
thought before deciding to go that way.

Cheers,
Jói


2011/9/24 Marc-Antoine Ruel <maruel@chromium.org>
>
> The main problem is that MSVC PCH handler doesn't handle changes in command
line defines safely. Like I pasted, it generates a warning C4651, treated as
error, instead of doing the right thing and regenerating the PCH when project
level defines change.
> M-A
>
> Le 24 septembre 2011 16:08, Jói Sigurðsson <joi@chromium.org> a écrit :
>>
>> BTW I already turned off use of precompiled headers in release builds to
avoid OOM problems in Canary builds.
>>
>> (sent from phone, pardon brevity)
>>
>> On Sep 24, 2011 7:05 PM, "Jói Sigurðsson" <joi@chromium.org> wrote:
>> >> But how come precompiled headers are used? Joi, we should disable that,
>> >> that's a
>> >> recipe for pain and suffering in infrastructure.
>> >
>> > I'd like to understand more about the pain, so that we can make the
>> > right tradeoff. Using preceompiled headers currently speeds up builds
>> > by about 18% from what they were before (for the 'chrome' target,
>> > maybe more when building all targets) and are likely to speed up the
>> > build by a total of about 25% once I make a similar change to WebKit
>> > to use precompiled headers (when building the same target). So the
>> > upside for keeping it on is considerable for developers and for bot
>> > cycle times.
>> >
>> > Are there situations other than the current one that make this painful
>> > for infrastructure? I think the current issue shouldn't come up more
>> > than a handful of times per year?
>> >
>> > Cheers,
>> > Jói
>> >
>> >
>> > On Sat, Sep 24, 2011 at 3:14 AM, <maruel@chromium.org> wrote:
>> >> Looks like it breaks incremental compile.
>> >>
>> >>
http://build.chromium.org/p/tryserver.chromium/builders/win_sync/builds/5545/...
>> >>
>> >> with errors like:
>> >>
>> >> e:\b\build\slave\win\build\src\base\command_line.cc : warning C4651:
>> >> '/D_WIN32_WINNT=0x0601' specified for precompiled header but not for
current
>> >> compile
>> >>
>> >> But how come precompiled headers are used? Joi, we should disable that,
>> >> that's a
>> >> recipe for pain and suffering in infrastructure.
>> >>
>> >> http://codereview.chromium.org/8004004/
>> >>
>

[jschuh, 2011-09-25 00:14:01]

Not to be a selfish jerk, but for the purposes of this CL the solution appears
to be a clobber build (which is what I already did locally and planned on
coordinating with the sheriffs). So, is it okay to move the discussion on PCH
out of this review?

[brettw, 2011-09-25 06:13:26]

http://codereview.chromium.org/8004004/diff/4007/net/base/test_root_certs_win.cc
File net/base/test_root_certs_win.cc (right):

http://codereview.chromium.org/8004004/diff/4007/net/base/test_root_certs_win...
net/base/test_root_certs_win.cc:179: const DWORD kSizeofCertChainEngineConfig =
Don't give this "k" naming since it's not a constant (it's based on the current
windows version). So sizeof_cert_chain_engine_config.


Actually, do we even need this? It looks like this just uses the default values
for everything. Can we always use the Pre-Win7 mode? It seems like Windows
already will have this exact same check inside the implemetation of the
function.

[jschuh, 2011-09-25 20:39:34]

http://codereview.chromium.org/8004004/diff/4007/net/base/test_root_certs_win.cc
File net/base/test_root_certs_win.cc (right):

http://codereview.chromium.org/8004004/diff/4007/net/base/test_root_certs_win...
net/base/test_root_certs_win.cc:179: const DWORD kSizeofCertChainEngineConfig =
On 2011/09/25 06:13:26, brettw wrote:
> Don't give this "k" naming since it's not a constant (it's based on the
current
> windows version). So sizeof_cert_chain_engine_config.
> 
> 
> Actually, do we even need this? It looks like this just uses the default
values
> for everything. Can we always use the Pre-Win7 mode? It seems like Windows
> already will have this exact same check inside the implemetation of the
> function.

I originally had it the other way and switched it when I noticed the existing
callers of the macro do version checking before-hand. However, you're correct
that we don't need anything introduced in the Win7 version here, so I'll just
switch it back to always using the pre Win7 version.

[brettw, 2011-09-26 15:40:00]

lgtm

[gavinp, 2011-09-26 16:04:25]

net LGTM