Parameterize the Google Update appid at build time instead of hard coding it ...

Parameterize the Google Update appid at build time instead of hard coding it in the source.

[bradn, 2009-08-28 07:33:43]

LGTM

http://codereview.chromium.org/178011/diff/1/6
File app/app.gyp (right):

http://codereview.chromium.org/178011/diff/1/6#newcode300
Line 300: },
Might be better to put this whole target in a condition, since it's windows only
correct?

http://codereview.chromium.org/178011/diff/1/6#newcode335
Line 335: 'process_outputs_as_sources': 1,
I don't think you need this, as headers don't count as sources in the sense of
having further build rules applied to them. But either way, doesn't hurt.

[kuchhal, 2009-08-28 17:03:37]

I think you will need to remove this guid from
chrome/installer/util/google_update_constants.cc as well.

http://codereview.chromium.org/178011/diff/1/8
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/178011/diff/1/8#newcode39
Line 39: #include "appid.h"
Can you check if this change causes the size of mini_installer.exe to change?

[kuchhal, 2009-08-28 17:04:25]

Ignore my previous comment. I saw that you already have
google_update_constants.cc in this change.

lgtm.

[robertshield, 2009-08-31 14:43:29]

Thanks!

http://codereview.chromium.org/178011/diff/1/6
File app/app.gyp (right):

http://codereview.chromium.org/178011/diff/1/6#newcode300
Line 300: },
On 2009/08/28 07:33:43, bradn wrote:
> Might be better to put this whole target in a condition, since it's windows
only
> correct?

I placed the 'actions' section inside a condition - placing the whole target
there caused errors (due to non-conditioned dependencies on the app_id target).

http://codereview.chromium.org/178011/diff/1/8
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/178011/diff/1/8#newcode39
Line 39: #include "appid.h"
On 2009/08/28 17:03:37, kuchhal wrote:
> Can you check if this change causes the size of mini_installer.exe to change?

It does: I compared a locally built release version of mini_installer.exe with a
release version from the internal builder - after stripping out the compressed
resources my mini_installer.exe was 11776 bytes vs 11264 bytes (+512 bytes)
without the patch. 

Is this a big enough delta to be a problem?

[kuchhal, 2009-08-31 15:48:45]

+cpu

 Can you check if this change causes the size of mini_installer.exe to
>>
> change?
>
> It does: I compared a locally built release version of
> mini_installer.exe with a release version from the internal builder -
> after stripping out the compressed resources my mini_installer.exe was
> 11776 bytes vs 11264 bytes (+512 bytes) without the patch.
>
> Is this a big enough delta to be a problem?
>
>
> http://codereview.chromium.org/178011
>

Probably not. Carlos?

[cpu_(ooo_6.6-7.5), 2009-09-01 00:12:28]

512 extra bytes is ok.


On 2009/08/31 15:48:45, kuchhal wrote:
> +cpu
> 
>  Can you check if this change causes the size of mini_installer.exe to
> >>
> > change?
> >
> > It does: I compared a locally built release version of
> > mini_installer.exe with a release version from the internal builder -
> > after stripping out the compressed resources my mini_installer.exe was
> > 11776 bytes vs 11264 bytes (+512 bytes) without the patch.
> >
> > Is this a big enough delta to be a problem?
> >
> >
> > http://codereview.chromium.org/178011
> >
> 
> Probably not. Carlos?
>

[sgk, 2009-09-03 00:25:23]

http://codereview.chromium.org/178011/diff/3003/5006
File app/app.gyp (right):

http://codereview.chromium.org/178011/diff/3003/5006#newcode330
Line 330: 'tools/build/_always_run_appid_py.marker',
There's a problem here.

You're using a .marker file to make sure that the appid update runs every single
time you build.  That's fine, but... (see next comment...)

http://codereview.chromium.org/178011/diff/3003/5004
File chrome/tools/build/appid.py (right):

http://codereview.chromium.org/178011/diff/3003/5004#newcode27
Line 27: output_file.write(contents)
...you're writing out the .h file contents *every time* even if the id is the
same as last time.  This is causing the timestamp on the generated .h file to
change every time, which causes installer_util to recompile
google_update_constants.cc every time, which causes chrome.dll to relink *every*
build, even when nothing has changed.

When you're running an action like this every build, you *must* compare the
output you would generate against the current contents of file, and only write
it out if there's a difference.

I'm prepping a CL to fix this.

[M-A Ruel, 2009-09-03 21:15:46]

You forgot to update svn:ignore. Will commit as soon as the tree opens.

[Finnur, 2009-09-04 21:15:17]

> http://codereview.chromium.org/178011/diff/3003/5004#newcode27
> Line 27: output_file.write(contents)
> ...you're writing out the .h file contents *every time* even if the id
> is the same as last time.  This is causing the timestamp on the
> generated .h file to change every time, which causes installer_util to
> recompile google_update_constants.cc every time, which causes chrome.dll
> to relink *every* build, even when nothing has changed.

> When you're running an action like this every build, you *must* compare
> the output you would generate against the current contents of file, and
> only write it out if there's a difference.

> I'm prepping a CL to fix this.
Any word on a CL to fix this? This problem is really annoying... :)


On Wed, Sep 2, 2009 at 17:25, <sgk@chromium.org> wrote:

>
>
> http://codereview.chromium.org/178011/diff/3003/5006
> File app/app.gyp (right):
>
> http://codereview.chromium.org/178011/diff/3003/5006#newcode330
> Line 330: 'tools/build/_always_run_appid_py.marker',
> There's a problem here.
>
> You're using a .marker file to make sure that the appid update runs
> every single time you build.  That's fine, but... (see next comment...)
>
> http://codereview.chromium.org/178011/diff/3003/5004
> File chrome/tools/build/appid.py (right):
>
> http://codereview.chromium.org/178011/diff/3003/5004#newcode27
> Line 27: output_file.write(contents)
> ...you're writing out the .h file contents *every time* even if the id
> is the same as last time.  This is causing the timestamp on the
> generated .h file to change every time, which causes installer_util to
> recompile google_update_constants.cc every time, which causes chrome.dll
> to relink *every* build, even when nothing has changed.
>
> When you're running an action like this every build, you *must* compare
> the output you would generate against the current contents of file, and
> only write it out if there's a difference.
>
> I'm prepping a CL to fix this.
>
>
> http://codereview.chromium.org/178011
>

[sgk1, 2009-09-04 22:15:38]

A fix for this was submitted, r25290:
http://codereview.chromium.org/199001/show

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

You should see appid run every time, but it shouldn't cause any downstream
rebuilds (specifically of chrome.dll).

If you're still seeing unnecessary rebuilds after r25290, it's probably due
to some other dependency problem.  Let me know the specifics (build log,
what target, etc.) and I can look into it.

        --SK

On Fri, Sep 4, 2009 at 2:14 PM, Finnur Thorarinsson <finnur@chromium.org>wrote:

>
> > http://codereview.chromium.org/178011/diff/3003/5004#newcode27
> > Line 27: output_file.write(contents)
> > ...you're writing out the .h file contents *every time* even if the id
> > is the same as last time.  This is causing the timestamp on the
> > generated .h file to change every time, which causes installer_util to
> > recompile google_update_constants.cc every time, which causes chrome.dll
> > to relink *every* build, even when nothing has changed.
>
> > When you're running an action like this every build, you *must* compare
> > the output you would generate against the current contents of file, and
> > only write it out if there's a difference.
>
> > I'm prepping a CL to fix this.
> Any word on a CL to fix this? This problem is really annoying... :)
>
>
>
> On Wed, Sep 2, 2009 at 17:25, <sgk@chromium.org> wrote:
>
>>
>>
>> http://codereview.chromium.org/178011/diff/3003/5006
>> File app/app.gyp (right):
>>
>> http://codereview.chromium.org/178011/diff/3003/5006#newcode330
>> Line 330: 'tools/build/_always_run_appid_py.marker',
>> There's a problem here.
>>
>> You're using a .marker file to make sure that the appid update runs
>> every single time you build.  That's fine, but... (see next comment...)
>>
>> http://codereview.chromium.org/178011/diff/3003/5004
>> File chrome/tools/build/appid.py (right):
>>
>> http://codereview.chromium.org/178011/diff/3003/5004#newcode27
>> Line 27: output_file.write(contents)
>> ...you're writing out the .h file contents *every time* even if the id
>> is the same as last time.  This is causing the timestamp on the
>> generated .h file to change every time, which causes installer_util to
>> recompile google_update_constants.cc every time, which causes chrome.dll
>> to relink *every* build, even when nothing has changed.
>>
>> When you're running an action like this every build, you *must* compare
>> the output you would generate against the current contents of file, and
>> only write it out if there's a difference.
>>
>> I'm prepping a CL to fix this.
>>
>>
>> http://codereview.chromium.org/178011
>>
>
>

[cpu_(ooo_6.6-7.5), 2009-09-18 18:09:54]

Can we close this issue?

On 2009/09/04 22:15:38, sgk_google.com wrote:
> A fix for this was submitted, r25290:
> http://codereview.chromium.org/199001/show
> 
> http://src.chromium.org/viewvc/chrome?view=rev&revision=25290
> 
> You should see appid run every time, but it shouldn't cause any downstream
> rebuilds (specifically of chrome.dll).
> 
> If you're still seeing unnecessary rebuilds after r25290, it's probably due
> to some other dependency problem.  Let me know the specifics (build log,
> what target, etc.) and I can look into it.
> 
>         --SK
> 
> On Fri, Sep 4, 2009 at 2:14 PM, Finnur Thorarinsson
<finnur@chromium.org>wrote:
> 
> >
> > > http://codereview.chromium.org/178011/diff/3003/5004#newcode27
> > > Line 27: output_file.write(contents)
> > > ...you're writing out the .h file contents *every time* even if the id
> > > is the same as last time.  This is causing the timestamp on the
> > > generated .h file to change every time, which causes installer_util to
> > > recompile google_update_constants.cc every time, which causes chrome.dll
> > > to relink *every* build, even when nothing has changed.
> >
> > > When you're running an action like this every build, you *must* compare
> > > the output you would generate against the current contents of file, and
> > > only write it out if there's a difference.
> >
> > > I'm prepping a CL to fix this.
> > Any word on a CL to fix this? This problem is really annoying... :)
> >
> >
> >
> > On Wed, Sep 2, 2009 at 17:25, <mailto:sgk@chromium.org> wrote:
> >
> >>
> >>
> >> http://codereview.chromium.org/178011/diff/3003/5006
> >> File app/app.gyp (right):
> >>
> >> http://codereview.chromium.org/178011/diff/3003/5006#newcode330
> >> Line 330: 'tools/build/_always_run_appid_py.marker',
> >> There's a problem here.
> >>
> >> You're using a .marker file to make sure that the appid update runs
> >> every single time you build.  That's fine, but... (see next comment...)
> >>
> >> http://codereview.chromium.org/178011/diff/3003/5004
> >> File chrome/tools/build/appid.py (right):
> >>
> >> http://codereview.chromium.org/178011/diff/3003/5004#newcode27
> >> Line 27: output_file.write(contents)
> >> ...you're writing out the .h file contents *every time* even if the id
> >> is the same as last time.  This is causing the timestamp on the
> >> generated .h file to change every time, which causes installer_util to
> >> recompile google_update_constants.cc every time, which causes chrome.dll
> >> to relink *every* build, even when nothing has changed.
> >>
> >> When you're running an action like this every build, you *must* compare
> >> the output you would generate against the current contents of file, and
> >> only write it out if there's a difference.
> >>
> >> I'm prepping a CL to fix this.
> >>
> >>
> >> http://codereview.chromium.org/178011
> >>
> >
> >
>

[bradn, 2009-09-18 18:10:57]

I think so.
-BradN

On Fri, Sep 18, 2009 at 11:09 AM, <cpu@chromium.org> wrote:

> Can we close this issue?
>
> On 2009/09/04 22:15:38, sgk_google.com wrote:
>
>> A fix for this was submitted, r25290:
>> http://codereview.chromium.org/199001/show
>>
>
>  http://src.chromium.org/viewvc/chrome?view=rev&revision=25290
>>
>
>  You should see appid run every time, but it shouldn't cause any downstream
>> rebuilds (specifically of chrome.dll).
>>
>
>  If you're still seeing unnecessary rebuilds after r25290, it's probably
>> due
>> to some other dependency problem.  Let me know the specifics (build log,
>> what target, etc.) and I can look into it.
>>
>
>          --SK
>>
>
>  On Fri, Sep 4, 2009 at 2:14 PM, Finnur Thorarinsson
>>
> <finnur@chromium.org>wrote:
>
>  >
>> > > http://codereview.chromium.org/178011/diff/3003/5004#newcode27
>> > > Line 27: output_file.write(contents)
>> > > ...you're writing out the .h file contents *every time* even if the id
>> > > is the same as last time.  This is causing the timestamp on the
>> > > generated .h file to change every time, which causes installer_util to
>> > > recompile google_update_constants.cc every time, which causes
>> chrome.dll
>> > > to relink *every* build, even when nothing has changed.
>> >
>> > > When you're running an action like this every build, you *must*
>> compare
>> > > the output you would generate against the current contents of file,
>> and
>> > > only write it out if there's a difference.
>> >
>> > > I'm prepping a CL to fix this.
>> > Any word on a CL to fix this? This problem is really annoying... :)
>> >
>> >
>> >
>> > On Wed, Sep 2, 2009 at 17:25, <mailto:sgk@chromium.org> wrote:
>> >
>> >>
>> >>
>> >> http://codereview.chromium.org/178011/diff/3003/5006
>> >> File app/app.gyp (right):
>> >>
>> >> http://codereview.chromium.org/178011/diff/3003/5006#newcode330
>> >> Line 330: 'tools/build/_always_run_appid_py.marker',
>> >> There's a problem here.
>> >>
>> >> You're using a .marker file to make sure that the appid update runs
>> >> every single time you build.  That's fine, but... (see next comment...)
>> >>
>> >> http://codereview.chromium.org/178011/diff/3003/5004
>> >> File chrome/tools/build/appid.py (right):
>> >>
>> >> http://codereview.chromium.org/178011/diff/3003/5004#newcode27
>> >> Line 27: output_file.write(contents)
>> >> ...you're writing out the .h file contents *every time* even if the id
>> >> is the same as last time.  This is causing the timestamp on the
>> >> generated .h file to change every time, which causes installer_util to
>> >> recompile google_update_constants.cc every time, which causes
>> chrome.dll
>> >> to relink *every* build, even when nothing has changed.
>> >>
>> >> When you're running an action like this every build, you *must* compare
>> >> the output you would generate against the current contents of file, and
>> >> only write it out if there's a difference.
>> >>
>> >> I'm prepping a CL to fix this.
>> >>
>> >>
>> >> http://codereview.chromium.org/178011
>> >>
>> >
>> >
>>
>
>
>
>
> http://codereview.chromium.org/178011
>