Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1641413002:
Makes GetBuildTime behave identically on all build types. (Closed)
|
Created:
4 years, 10 months ago by Zachary Forman Modified:
4 years, 10 months ago CC:
chromium-reviews, mithro-old, Sébastien Marchand, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMakes GetBuildTime behave sanely on all build types.
After discussion with maruel and agl, it seems that
(1) for the purposes of build determinism, it's necessary
to be able to arbitrarily set the build time.
(2) for the purposes of continuous integration, longer duration
between cache invalidation is better, but >=1mo is preferable.
(3) for security purposes, timebombs would ideally be as close to
the actual time of the build as possible. It must be in the past.
(4) HSTS certificate pinning is valid for 70 days. To make CI builds enforce
HTST pinning, <=1mo is preferable.
All of these can reasonably be satisfied by using different settings for CI
versus official builds:
- For official build, the build time is set to 5:00am of the day of the build or
the day before.
- For continuous integration build, the build time is set to the current month.
If the current day is within the first week of the month and last Sunday
wasn't part of the current month, the Sunday of the previous month is used.
This results that cache invalidation happens on a Sunday, which is preferable
from an infrastructure standpoint.
- In the case that the build time needs to be set to a specific value (i.e. to
reproduce a build), the GN/GYP variable 'override_build_date' can be used to
set the BUILD_DATE explicitly. Its format is "Mmm DD YYYY".
The way it is done is:
- Generate $target_gen_dir/generated_build_date.h that defines BUILD_DATE. Its
value depends on if an official build is done or not.
- This step depends on build/util/LASTCHANGE so it is run at every sync. The
file is only touched if the content changed to not affect null build.
Most importantly, this change removes the need of both GN/GYP variable
"dont_embed_build_metadata" and C define "DONT_EMBED_BUILD_METADATA"; the build
is always deterministic (up to a month) by default. This removes the risk
oversight of forgetting to set this variable, which already happened.
R=maruel@chromium.org
BUG=489490
Committed: https://crrev.com/08d91b75212b6592f05ff993d5a71c0f5a546563
Cr-Commit-Position: refs/heads/master@{#375136}
Patch Set 1 #Patch Set 2 : Removes other checks #Patch Set 3 : Follows rename. #
Total comments: 17
Patch Set 4 : Uses python to generate header #Patch Set 5 : And adds the necessary config to gyp... #Patch Set 6 : Fixed bad input #
Total comments: 19
Patch Set 7 : Responses to comments #Patch Set 8 : Tests for build time < now invariant #Patch Set 9 : Addresses Merge Conflicts #
Total comments: 21
Patch Set 10 : Python tidyup, response to comments #Patch Set 11 : Hook based approach #
Total comments: 6
Patch Set 12 : Go back to gn/ninja solution. Remove instances of dont_use_build_metadata. #Patch Set 13 : Go back to gn/ninja solution. Remove instances of dont_use_build_metadata. #
Total comments: 12
Patch Set 14 : Response to comments #
Total comments: 4
Patch Set 15 : Response to dpranke's comments (#79) #Patch Set 16 : Adds missing dependency to base_nacl and co #Patch Set 17 : Possible fix for windows build error #
Total comments: 9
Patch Set 18 : Addresses comments (#85-#89) #
Total comments: 4
Patch Set 19 : Response to comments + fixes gyp build error. #
Total comments: 12
Patch Set 20 : Response to #96 #
Total comments: 1
Messages
Total messages: 118 (19 generated)
Description was changed from
==========
Makes GetBuildTime behave identically on all build types.
After discussion with maruel and agl, it seems that
(1) for the purposes of build determinism, it's necessary
to be able to arbitrarily set the build time.
(2) for the purposes of continuous integration, longer durations
between cache invalidation are better, but 24h is probably
workable.
(3) for security purposes, timebombs would ideally be as close to
the actual time of the build as possible.
All of these can reasonably be satisfied by setting the build time
to midnight the day of the build. In the case that the build time needs
to be set to a specific value (i.e. to reproduce a build), a define can
be used to set the BUILD_DATE explicitly.
There still exist a number of checks against DONT_EMBED_BUILD_METADATA
at locations where GetBuildTime() is frequently used - I'm not familiar
enough with the contexts where that code is used to say with surety if
those should be removed or left as is.
While this doesn't actually store current build time in a resource, it
answers the spirit of the bug below - it makes it possible to run that
type of test locally without setting a special flag.
BUG=489490
==========
to
==========
Makes GetBuildTime behave identically on all build types.
After discussion with maruel and agl, it seems that
(1) for the purposes of build determinism, it's necessary
to be able to arbitrarily set the build time.
(2) for the purposes of continuous integration, longer durations
between cache invalidation are better, but 24h is probably
workable.
(3) for security purposes, timebombs would ideally be as close to
the actual time of the build as possible.
All of these can reasonably be satisfied by setting the build time
to midnight the day of the build. In the case that the build time needs
to be set to a specific value (i.e. to reproduce a build), a define can
be used to set the BUILD_DATE explicitly.
There still exist a number of checks against DONT_EMBED_BUILD_METADATA
at locations where GetBuildTime() is frequently used - I'm not familiar
enough with the contexts where that code is used to say with surety if
those should be removed. In the meantime, I've removed these, however -
if you know of a reason why this is unreasonable, let me know.
While this doesn't actually store current build time in a resource, it
answers the spirit of the bug below - it makes it possible to run that
type of test locally without setting a special flag.
BUG=489490
==========
zforman@google.com changed reviewers: + agl@chromium.org, maruel@chromium.org
Hey all - could you please take a look at this CL? Thanks, Zac
https://codereview.chromium.org/1641413002/diff/40001/base/build_time.h File base/build_time.h (right): https://codereview.chromium.org/1641413002/diff/40001/base/build_time.h#newco... base/build_time.h:14: // rounded to midnight at the start of the day. s/rounded/rounded down/ (Normal rounding would suggest that it went to the nearest midnight, which might be the future.) s/ the day/ the day in UTC/ https://codereview.chromium.org/1641413002/diff/40001/base/build_time.h#newco... base/build_time.h:19: // This value should only be considered accurate to a day. s/a day/36 hours/ If you're just to the east of the dateline then you'll be the last in the world to see the new day. Your build can then get stamped with the date (in your local timezone) at midnight UTC, which is wrong by 36 hours. https://codereview.chromium.org/1641413002/diff/40001/base/build_time_unittes... File base/build_time_unittest.cc (right): https://codereview.chromium.org/1641413002/diff/40001/base/build_time_unittes... base/build_time_unittest.cc:10: char build_date[] = __DATE__; I'm not sure when DONT_EMBED_BUILD_METADATA was defined but this test could fail for people when building with components. But if they weren't building with that define before then it'll be no worse. Possibly nothing to be done here. https://codereview.chromium.org/1641413002/diff/40001/components/ssl_errors/e... File components/ssl_errors/error_classification.cc (left): https://codereview.chromium.org/1641413002/diff/40001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:215: #if defined(DONT_EMBED_BUILD_METADATA) && !defined(OFFICIAL_BUILD) This (and below) needs careful consideration about when DONT_EMBED_BUILD_METADATA was defined. You might need felt or estark to sign off on this. https://codereview.chromium.org/1641413002/diff/40001/net/cert/ct_policy_enfo... File net/cert/ct_policy_enforcer.cc (left): https://codereview.chromium.org/1641413002/diff/40001/net/cert/ct_policy_enfo... net/cert/ct_policy_enforcer.cc:40: #if defined(DONT_EMBED_BUILD_METADATA) && !defined(OFFICIAL_BUILD) ditto. https://codereview.chromium.org/1641413002/diff/40001/net/http/transport_secu... File net/http/transport_security_state.cc (left): https://codereview.chromium.org/1641413002/diff/40001/net/http/transport_secu... net/http/transport_security_state.cc:1017: #if defined(DONT_EMBED_BUILD_METADATA) && !defined(OFFICIAL_BUILD) ditto.
https://codereview.chromium.org/1641413002/diff/40001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1641413002/diff/40001/base/BUILD.gn#newcode30 base/BUILD.gn:30: override_build_date = "N/A" What about: - By default override_build_date is set to the first of the month. This means normal builds will be deterministic for the month only. This means that on the first of each month, the cache will be invalidated, causing significant load. :/ - On official build, override_build_date defaults to "N/A". https://codereview.chromium.org/1641413002/diff/40001/base/build_time.cc File base/build_time.cc (right): https://codereview.chromium.org/1641413002/diff/40001/base/build_time.cc#newc... base/build_time.cc:19: // The format of __DATE__ and __TIME__ is specified by the ANSI C Standard, Remove the comment about __TIME__, it's not used. https://codereview.chromium.org/1641413002/diff/40001/base/build_time.cc#newc... base/build_time.cc:23: // __TIME__ is exactly "hh:mm:ss". Remove this, this is confusing. https://codereview.chromium.org/1641413002/diff/40001/base/build_time.h File base/build_time.h (right): https://codereview.chromium.org/1641413002/diff/40001/base/build_time.h#newco... base/build_time.h:14: // rounded to midnight at the start of the day. On 2016/01/29 21:46:45, agl wrote: > s/rounded/rounded down/ > > (Normal rounding would suggest that it went to the nearest midnight, which might > be the future.) > > s/ the day/ the day in UTC/ of the day in the current time zone, since __DATE__ is time local. https://codereview.chromium.org/1641413002/diff/40001/base/build_time_unittes... File base/build_time_unittest.cc (right): https://codereview.chromium.org/1641413002/diff/40001/base/build_time_unittes... base/build_time_unittest.cc:10: char build_date[] = __DATE__; On 2016/01/29 21:46:46, agl wrote: > I'm not sure when DONT_EMBED_BUILD_METADATA was defined but this test could fail > for people when building with components. But if they weren't building with that > define before then it'll be no worse. Possibly nothing to be done here. It's meant to be used on the try server. We want to have no dependence on the build time when building on the try server, so results can be reused. This is significant, we're talking of almost 50% of tests skipped due to this on linux. On the other hand, I'm wiling to take a performance hit to reduce the configuration knobs. Similar to what was proposed earlier over email: - Official build is precise to the resolution of +/-1 day + time zone. - Non official build is precise at [0~-30]day + time zone. This way, "chromium" builds would still have some security features enabled but for shorter. 30 would have to be less than half of the number of days after a build where the internal table is considered not valid anymore. If the table is good for 70 days, a -30 day variance we should be just fine. I'm purely talking about non-official build here, for try server, commit queue and continuous integration testing.
zforman@google.com changed reviewers: + felt@chromium.org, thakis@chromium.org
I've modified this CL to use a python script to generate a build date based on the type of the build (with 1 day / 30 day accuracy for official / non-official builds). Currently, this is causing failures on the windows trybots (unsure why so far, will look at tomorrow morning) (the mac failure seems unrelated). I've also added felt@ to this CL - could you please take a look at components/ssl_errors/error_classification.cc, net/cert/ct_policy_enforcer.cc, and net/http/transport_security_state.cc, as agl@ suggests in his comments on patchset 3. https://codereview.chromium.org/1641413002/diff/40001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1641413002/diff/40001/base/BUILD.gn#newcode30 base/BUILD.gn:30: override_build_date = "N/A" On 2016/01/30 at 02:58:14, M-A Ruel wrote: > What about: > > - By default override_build_date is set to the first of the month. This means normal builds will be deterministic for the month only. This means that on the first of each month, the cache will be invalidated, causing significant load. :/ > - On official build, override_build_date defaults to "N/A". I like this - we could actually use a python script, and actually use 'real' UTC, as well. https://codereview.chromium.org/1641413002/diff/40001/base/build_time.cc File base/build_time.cc (right): https://codereview.chromium.org/1641413002/diff/40001/base/build_time.cc#newc... base/build_time.cc:19: // The format of __DATE__ and __TIME__ is specified by the ANSI C Standard, On 2016/01/30 at 02:58:14, M-A Ruel wrote: > Remove the comment about __TIME__, it's not used. Done https://codereview.chromium.org/1641413002/diff/40001/base/build_time.cc#newc... base/build_time.cc:23: // __TIME__ is exactly "hh:mm:ss". On 2016/01/30 at 02:58:14, M-A Ruel wrote: > Remove this, this is confusing. Done https://codereview.chromium.org/1641413002/diff/40001/base/build_time.h File base/build_time.h (right): https://codereview.chromium.org/1641413002/diff/40001/base/build_time.h#newco... base/build_time.h:14: // rounded to midnight at the start of the day. On 2016/01/30 at 02:58:14, M-A Ruel wrote: > On 2016/01/29 21:46:45, agl wrote: > > s/rounded/rounded down/ > > > > (Normal rounding would suggest that it went to the nearest midnight, which might > > be the future.) > > > > s/ the day/ the day in UTC/ > > of the day in the current time zone, since __DATE__ is time local. Done. I've adjusted it to use the result of a python script, so we have no dependency on the time zone that we're currently building in :). https://codereview.chromium.org/1641413002/diff/40001/base/build_time.h#newco... base/build_time.h:19: // This value should only be considered accurate to a day. On 2016/01/29 at 21:46:46, agl wrote: > s/a day/36 hours/ > > If you're just to the east of the dateline then you'll be the last in the world to see the new day. Your build can then get stamped with the date (in your local timezone) at midnight UTC, which is wrong by 36 hours. Done. I've made it always use the real time at UTC, and hence narrowed this back down to 24h. https://codereview.chromium.org/1641413002/diff/40001/base/build_time_unittes... File base/build_time_unittest.cc (right): https://codereview.chromium.org/1641413002/diff/40001/base/build_time_unittes... base/build_time_unittest.cc:10: char build_date[] = __DATE__; On 2016/01/30 at 02:58:14, M-A Ruel wrote: > On 2016/01/29 21:46:46, agl wrote: > > I'm not sure when DONT_EMBED_BUILD_METADATA was defined but this test could fail > > for people when building with components. But if they weren't building with that > > define before then it'll be no worse. Possibly nothing to be done here. > > It's meant to be used on the try server. > > We want to have no dependence on the build time when building on the try server, so results can be reused. This is significant, we're talking of almost 50% of tests skipped due to this on linux. > > On the other hand, I'm wiling to take a performance hit to reduce the configuration knobs. > > Similar to what was proposed earlier over email: > - Official build is precise to the resolution of +/-1 day + time zone. > - Non official build is precise at [0~-30]day + time zone. > > This way, "chromium" builds would still have some security features enabled but for shorter. > > 30 would have to be less than half of the number of days after a build where the internal table is considered not valid anymore. If the table is good for 70 days, a -30 day variance we should be just fine. I'm purely talking about non-official build here, for try server, commit queue and continuous integration testing. This sounds pretty reasonable to me. I've implemented this with a python script, so we get 'real' GMT times.
Description was changed from
==========
Makes GetBuildTime behave identically on all build types.
After discussion with maruel and agl, it seems that
(1) for the purposes of build determinism, it's necessary
to be able to arbitrarily set the build time.
(2) for the purposes of continuous integration, longer durations
between cache invalidation are better, but 24h is probably
workable.
(3) for security purposes, timebombs would ideally be as close to
the actual time of the build as possible.
All of these can reasonably be satisfied by setting the build time
to midnight the day of the build. In the case that the build time needs
to be set to a specific value (i.e. to reproduce a build), a define can
be used to set the BUILD_DATE explicitly.
There still exist a number of checks against DONT_EMBED_BUILD_METADATA
at locations where GetBuildTime() is frequently used - I'm not familiar
enough with the contexts where that code is used to say with surety if
those should be removed. In the meantime, I've removed these, however -
if you know of a reason why this is unreasonable, let me know.
While this doesn't actually store current build time in a resource, it
answers the spirit of the bug below - it makes it possible to run that
type of test locally without setting a special flag.
BUG=489490
==========
to
==========
Makes GetBuildTime behave identically on all build types.
After discussion with maruel and agl, it seems that
(1) for the purposes of build determinism, it's necessary
to be able to arbitrarily set the build time.
(2) for the purposes of continuous integration, longer durations
between cache invalidation are better, but 24h is probably
workable.
(3) for security purposes, timebombs would ideally be as close to
the actual time of the build as possible.
All of these can reasonably be satisfied by setting the build time
to midnight the day of the build. In the case that the build time needs
to be set to a specific value (i.e. to reproduce a build), a define can
be used to set the BUILD_DATE explicitly.
There still exist a number of checks against DONT_EMBED_BUILD_METADATA
at locations where GetBuildTime() is frequently used - I'm not familiar
enough with the contexts where that code is used to say with surety if
those should be removed. In the meantime, I've removed these, however -
if you know of a reason why this is unreasonable, let me know.
While this doesn't actually store current build time in a resource, it
answers the spirit of the bug below - it makes it possible to run that
type of test locally without setting a special flag.
BUG=489490
==========
There's also third_party/yasm/CHROMIUM.diff that references DONT_EMBED_BUILD_METADATA. This means we'll have to update this third party. We should just get rid of the __DATE__ reference unconditionally. https://codereview.chromium.org/1641413002/diff/100001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1641413002/diff/100001/base/BUILD.gn#newcode1996 base/BUILD.gn:1996: args += [ "--override" ] Does this work? args += [ "--override", override_build_date ] https://codereview.chromium.org/1641413002/diff/100001/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/1641413002/diff/100001/base/base.gyp#newcode1119 base/base.gyp:1119: 'build_type': '--official' --default ? https://codereview.chromium.org/1641413002/diff/100001/build/write_build_date... File build/write_build_date_header.py (right): https://codereview.chromium.org/1641413002/diff/100001/build/write_build_date... build/write_build_date_header.py:6: # Writes a file that contains a define that approximates the build date. Convert to docstring. https://codereview.chromium.org/1641413002/diff/100001/build/write_build_date... build/write_build_date_header.py:14: 2 lines between file level symbols. https://codereview.chromium.org/1641413002/diff/100001/build/write_build_date... build/write_build_date_header.py:16: if build_type == "--official": use single quotes everywhere. https://codereview.chromium.org/1641413002/diff/100001/build/write_build_date... build/write_build_date_header.py:19: format = "{:%b 01 %Y}" I thought about it more and I'd want a slightly more complex algorithm. I'd want it to switch on Saturday, so that the cold cache rush (first of the month) never happens during a week day. To do so, you'll have to change the algorithm to look at the current month, and if it's between Sunday and Saturday and on the first week of the month, it should return the month before. Does it makes sense? https://codereview.chromium.org/1641413002/diff/100001/build/write_build_date... build/write_build_date_header.py:23: output_location = arguments[1]; No trailing ; https://codereview.chromium.org/1641413002/diff/100001/build/write_build_date... build/write_build_date_header.py:24: build_type = arguments[2] if len(arguments) > 2 else "default" Please use argparse. It's going to make the script almost 2x longer but it removes the risk of a parsing error.
felt@chromium.org changed reviewers: + lgarron@chromium.org
I'm excited to have clock interstitial working in local builds! :-D IsUserClockInThePast() can definitely use the rounded build time. However, the time needs to stay in the past. I've left a comment about that. https://codereview.chromium.org/1641413002/diff/100001/base/build_time.h File base/build_time.h (right): https://codereview.chromium.org/1641413002/diff/100001/base/build_time.h#newc... base/build_time.h:19: // This value should only be considered accurate to within a day. Unless it's off by a month. ;-) Would you mind documenting and enforcing a guarantee that the returned time is in the past, relative to the computer running the build? IsUserClockInThePast() it requires that the build time is in the past, otherwise you will get strange results [1]. It seems that the CT code would also prefer to avoid a build date in the past. (If "in the past" is too strong, "at most one day ahead" is probably fine.) In particular, could you add a unit test with a check that `GetBuildTime() < __DATE__`? The test will require a recompile every time, but it's trading that recompile against security-sensitive correctness on the build speedup. [1] In particular, if the build date is at least 2 days the future, an expired certificate will show a "bad clock" warning even if your clock is fine. https://code.google.com/p/chromium/issues/detail?id=414843#c16
On 2016/02/02 at 04:03:11, lgarron wrote: > I'm excited to have clock interstitial working in local builds! :-D > > IsUserClockInThePast() can definitely use the rounded build time. However, the time needs to stay in the past. I've left a comment about that. > > https://codereview.chromium.org/1641413002/diff/100001/base/build_time.h > File base/build_time.h (right): > > https://codereview.chromium.org/1641413002/diff/100001/base/build_time.h#newc... > base/build_time.h:19: // This value should only be considered accurate to within a day. > Unless it's off by a month. ;-) > > Would you mind documenting and enforcing a guarantee that the returned time is in the past, relative to the computer running the build? IsUserClockInThePast() it requires that the build time is in the past, otherwise you will get strange results [1]. It seems that the CT code would also prefer to avoid a build date in the past. > (If "in the past" is too strong, "at most one day ahead" is probably fine.) > > In particular, could you add a unit test with a check that `GetBuildTime() < __DATE__`? > The test will require a recompile every time, but it's trading that recompile against security-sensitive correctness on the build speedup. > > [1] In particular, if the build date is at least 2 days the future, an expired certificate will show a "bad clock" warning even if your clock is fine. https://code.google.com/p/chromium/issues/detail?id=414843#c16 This sounds fairly reasonable to me. I'd rather not check that GetBuildTime() < __DATE__, but rather that GetBuildTime() < base::Time::Now(). As it turns out, base::Time::Now() returns the current time in UTC as well, so we'll /always/ observe a time that's in the same timezone as the build and after the build time provided that we use that (or some derivative of it) to check.
https://codereview.chromium.org/1641413002/diff/100001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1641413002/diff/100001/base/BUILD.gn#newcode1996 base/BUILD.gn:1996: args += [ "--override" ] On 2016/02/01 at 17:52:28, M-A Ruel wrote: > Does this work? > args += [ "--override", override_build_date ] Yeah, it should. Fixing! https://codereview.chromium.org/1641413002/diff/100001/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/1641413002/diff/100001/base/base.gyp#newcode1119 base/base.gyp:1119: 'build_type': '--official' On 2016/02/01 at 17:52:28, M-A Ruel wrote: > --default > ? Oops - good catch! https://codereview.chromium.org/1641413002/diff/100001/base/build_time.h File base/build_time.h (right): https://codereview.chromium.org/1641413002/diff/100001/base/build_time.h#newc... base/build_time.h:19: // This value should only be considered accurate to within a day. On 2016/02/02 at 04:03:11, lgarron wrote: > Unless it's off by a month. ;-) > > Would you mind documenting and enforcing a guarantee that the returned time is in the past, relative to the computer running the build? IsUserClockInThePast() it requires that the build time is in the past, otherwise you will get strange results [1]. It seems that the CT code would also prefer to avoid a build date in the past. > (If "in the past" is too strong, "at most one day ahead" is probably fine.) > > In particular, could you add a unit test with a check that `GetBuildTime() < __DATE__`? > The test will require a recompile every time, but it's trading that recompile against security-sensitive correctness on the build speedup. > > [1] In particular, if the build date is at least 2 days the future, an expired certificate will show a "bad clock" warning even if your clock is fine. https://code.google.com/p/chromium/issues/detail?id=414843#c16 Have done! https://codereview.chromium.org/1641413002/diff/100001/build/write_build_date... File build/write_build_date_header.py (right): https://codereview.chromium.org/1641413002/diff/100001/build/write_build_date... build/write_build_date_header.py:6: # Writes a file that contains a define that approximates the build date. On 2016/02/01 at 17:52:28, M-A Ruel wrote: > Convert to docstring. Done! https://codereview.chromium.org/1641413002/diff/100001/build/write_build_date... build/write_build_date_header.py:14: On 2016/02/01 at 17:52:28, M-A Ruel wrote: > 2 lines between file level symbols. Done! https://codereview.chromium.org/1641413002/diff/100001/build/write_build_date... build/write_build_date_header.py:16: if build_type == "--official": On 2016/02/01 at 17:52:28, M-A Ruel wrote: > use single quotes everywhere. Done! https://codereview.chromium.org/1641413002/diff/100001/build/write_build_date... build/write_build_date_header.py:19: format = "{:%b 01 %Y}" On 2016/02/01 at 17:52:28, M-A Ruel wrote: > I thought about it more and I'd want a slightly more complex algorithm. > > I'd want it to switch on Saturday, so that the cold cache rush (first of the month) never happens during a week day. > > To do so, you'll have to change the algorithm to look at the current month, and if it's between Sunday and Saturday and on the first week of the month, it should return the month before. Does it makes sense? I like this idea. I'd want it to switch on Sunday in GMT, so that it hits the US (at 8 hours behind) on a Saturday afternoon instead of a Friday afternoon, and Australia (on the other end) relatively early on a Sunday morning. https://codereview.chromium.org/1641413002/diff/100001/build/write_build_date... build/write_build_date_header.py:23: output_location = arguments[1]; On 2016/02/01 at 17:52:28, M-A Ruel wrote: > No trailing ; Done https://codereview.chromium.org/1641413002/diff/100001/build/write_build_date... build/write_build_date_header.py:24: build_type = arguments[2] if len(arguments) > 2 else "default" On 2016/02/01 at 17:52:28, M-A Ruel wrote: > Please use argparse. It's going to make the script almost 2x longer but it removes the risk of a parsing error. Done!
https://codereview.chromium.org/1641413002/diff/100001/base/build_time.h File base/build_time.h (right): https://codereview.chromium.org/1641413002/diff/100001/base/build_time.h#newc... base/build_time.h:19: // This value should only be considered accurate to within a day. On 2016/02/02 at 06:30:38, Zachary Forman wrote: > On 2016/02/02 at 04:03:11, lgarron wrote: > > Unless it's off by a month. ;-) > > > > Would you mind documenting and enforcing a guarantee that the returned time is in the past, relative to the computer running the build? IsUserClockInThePast() it requires that the build time is in the past, otherwise you will get strange results [1]. It seems that the CT code would also prefer to avoid a build date in the past. > > (If "in the past" is too strong, "at most one day ahead" is probably fine.) > > > > In particular, could you add a unit test with a check that `GetBuildTime() < __DATE__`? > > The test will require a recompile every time, but it's trading that recompile against security-sensitive correctness on the build speedup. > > > > [1] In particular, if the build date is at least 2 days the future, an expired certificate will show a "bad clock" warning even if your clock is fine. https://code.google.com/p/chromium/issues/detail?id=414843#c16 > > Have done! Thanks! I'm not a real owner, but LGTM for this test and for components/ssl_errors/error_classification.cc
lgtm per lgarron
maruel@chromium.org changed reviewers: + dpranke@chromium.org, laforge@chromium.org
+laforge for incremental build on official build +dpranke for the idea below Sorry for taking this long on this one, I really want to make sure we have the best long term solution here. My main concern is that the current implementation means we need to clobber to update the date. This is a concern. On the other hand, we want to keep the null build a null build, this is important. These two goals contradict each other. An option is to not use an header file and specify the define via CFLAGS by running a command while GN/GYP are running to calculate the date. This means that the date would be updated every time GN is run, which is a better moment. The drawback is slowing down GN / running a command via GN, which may not be acceptable (?) Dirk can answer this. https://codereview.chromium.org/1641413002/diff/160001/base/build_time.h File base/build_time.h (right): https://codereview.chromium.org/1641413002/diff/160001/base/build_time.h#newc... base/build_time.h:17: // changes. However, official builds will always be built from scratch. Technically, this is not 100% true. Adding Anthony so he can clarify this. https://codereview.chromium.org/1641413002/diff/160001/build/write_build_date... File build/write_build_date_header.py (right): https://codereview.chromium.org/1641413002/diff/160001/build/write_build_date... build/write_build_date_header.py:4: # found in the LICENSE file. ... # found in the LICENSE file. """Writes a file that contains a define that approximates the build date. The build date is set to the most recent first Sunday of a month, in UTC time. """ https://codereview.chromium.org/1641413002/diff/160001/build/write_build_date... build/write_build_date_header.py:10: from calendar import Calendar The style is to import only the package, then sort. https://codereview.chromium.org/1641413002/diff/160001/build/write_build_date... build/write_build_date_header.py:17: """ """Returns ... same everywhere. https://codereview.chromium.org/1641413002/diff/160001/build/write_build_date... build/write_build_date_header.py:21: weeks = cal.monthdays2calendar(year, month) weeks = calendar.Calendar().monthdays2calendar(year, month) https://codereview.chromium.org/1641413002/diff/160001/build/write_build_date... build/write_build_date_header.py:23: return [ date_day[0] for date_day in weeks[0] if date_day[1] == 6][0] return [date_day[0] ... https://codereview.chromium.org/1641413002/diff/160001/build/write_build_date... build/write_build_date_header.py:53: argument_parser.add_argument('build_type', help='The type of build') Use choices=(). https://codereview.chromium.org/1641413002/diff/160001/build/write_build_date... build/write_build_date_header.py:54: argument_parser.add_argument('build_date_override', nargs='?', default=None, Remove default=None, it's the default. https://codereview.chromium.org/1641413002/diff/160001/build/write_build_date... build/write_build_date_header.py:58: if args.build_date_override is not None: if args.build_date_override:
I don't understand the "this is a generated file, so it doesn't trigger a rebuild" comment. Why doesn't it? It should: The build system knows that a) the .h file is generated by your script b) the .cc file uses that .h file. Are you just missing a hard_dependency in gyp? https://codereview.chromium.org/1641413002/diff/160001/base/build_time.cc File base/build_time.cc (right): https://codereview.chromium.org/1641413002/diff/160001/base/build_time.cc#new... base/build_time.cc:18: const char kDateTime[] = BUILD_DATE " 00:00:00"; Can we make this 5am or something not right at the boundary?
On 2016/02/04 15:28:16, Nico wrote: > I don't understand the "this is a generated file, so it doesn't trigger a > rebuild" comment. Why doesn't it? It should: The build system knows that a) the > .h file is generated by your script b) the .cc file uses that .h file. Are you > just missing a hard_dependency in gyp? I'm concerned about incremental build which will get stuck at an old day because there's no way to invalidate that the header file is stale.
On 2016/02/04 15:30:31, M-A Ruel wrote: > On 2016/02/04 15:28:16, Nico wrote: > > I don't understand the "this is a generated file, so it doesn't trigger a > > rebuild" comment. Why doesn't it? It should: The build system knows that a) > the > > .h file is generated by your script b) the .cc file uses that .h file. Are you > > just missing a hard_dependency in gyp? > > I'm concerned about incremental build which will get stuck at an old day because > there's no way to invalidate that the header file is stale. But the build date is passed as a command-line argument to the script, right? And ninja tracks command lines and reruns commands whose command line changes.
On 2016/02/04 15:37:35, Nico wrote: > On 2016/02/04 15:30:31, M-A Ruel wrote: > > On 2016/02/04 15:28:16, Nico wrote: > > > I don't understand the "this is a generated file, so it doesn't trigger a > > > rebuild" comment. Why doesn't it? It should: The build system knows that a) > > the > > > .h file is generated by your script b) the .cc file uses that .h file. Are > you > > > just missing a hard_dependency in gyp? > > > > I'm concerned about incremental build which will get stuck at an old day > because > > there's no way to invalidate that the header file is stale. > > But the build date is passed as a command-line argument to the script, right? > And ninja tracks command lines and reruns commands whose command line changes. "write_build_date_header.py official" is a constant command yet its output changes every day. Same for "default" but the output only changes once per month.
oh, that's no good then, i agree.
https://codereview.chromium.org/1641413002/diff/160001/base/build_time.h File base/build_time.h (right): https://codereview.chromium.org/1641413002/diff/160001/base/build_time.h#newc... base/build_time.h:17: // changes. However, official builds will always be built from scratch. On 2016/02/04 15:24:13, M-A Ruel wrote: > Technically, this is not 100% true. Adding Anthony so he can clarify this. That's true for our full release builds, however for our continuous official builds those do not get clobbered.
Thanks for all the Python advice! Hopefully looking a lot better now :) https://codereview.chromium.org/1641413002/diff/160001/build/write_build_date... File build/write_build_date_header.py (right): https://codereview.chromium.org/1641413002/diff/160001/build/write_build_date... build/write_build_date_header.py:4: # found in the LICENSE file. On 2016/02/04 at 15:24:13, M-A Ruel wrote: > ... > # found in the LICENSE file. > > """Writes a file that contains a define that approximates the build date. > > The build date is set to the most recent first Sunday of a month, in UTC time. > """ Done https://codereview.chromium.org/1641413002/diff/160001/build/write_build_date... build/write_build_date_header.py:10: from calendar import Calendar On 2016/02/04 at 15:24:13, M-A Ruel wrote: > The style is to import only the package, then sort. Acknowledged and fixed. https://codereview.chromium.org/1641413002/diff/160001/build/write_build_date... build/write_build_date_header.py:17: """ On 2016/02/04 at 15:24:13, M-A Ruel wrote: > """Returns ... > > same everywhere. Done https://codereview.chromium.org/1641413002/diff/160001/build/write_build_date... build/write_build_date_header.py:21: weeks = cal.monthdays2calendar(year, month) On 2016/02/04 at 15:24:13, M-A Ruel wrote: > weeks = calendar.Calendar().monthdays2calendar(year, month) Done https://codereview.chromium.org/1641413002/diff/160001/build/write_build_date... build/write_build_date_header.py:23: return [ date_day[0] for date_day in weeks[0] if date_day[1] == 6][0] On 2016/02/04 at 15:24:13, M-A Ruel wrote: > return [date_day[0] ... Done https://codereview.chromium.org/1641413002/diff/160001/build/write_build_date... build/write_build_date_header.py:53: argument_parser.add_argument('build_type', help='The type of build') On 2016/02/04 at 15:24:13, M-A Ruel wrote: > Use choices=(). Done https://codereview.chromium.org/1641413002/diff/160001/build/write_build_date... build/write_build_date_header.py:54: argument_parser.add_argument('build_date_override', nargs='?', default=None, On 2016/02/04 at 15:24:13, M-A Ruel wrote: > Remove default=None, it's the default. Done https://codereview.chromium.org/1641413002/diff/160001/build/write_build_date... build/write_build_date_header.py:58: if args.build_date_override is not None: On 2016/02/04 at 15:24:13, M-A Ruel wrote: > if args.build_date_override: Done
dpranke@chromium.org changed reviewers: + brettw@chromium.org
I don't think I understand what problem we're trying to solve, but the approach that we're taking here seems wrong. I don't think embedding the build date into the binary is a good idea, because I don't think there's any way you can do so accurately without also making the build always be dirty (as already discussed in the comments on this CL). Trying to get around this by being kinda out of date and relying on clobbers to reset things and setting defines to reproduce things all feels like hacks piled on hacks. In addition, it seems like there are issues around what happens if clients aren't updated for long periods of time and the binaries become "stale"; what happens then? Here are a few other suggested approaches: 1) get the time from a server, not the client machine; I think we already have code to do this in sync (//components/network_time, maybe)? If we can't get the time from a server, because we need to validate certs before we connect to *any server ever*, here's some potential variants on the approach taken here that won't dirty the build: 2) just use the __DATE__ embedded in build_time.cc, but don't bother to mess with generated headers. The date will be updated whenever we recompile build_time.cc (you might still want a build_date arg for reproducibility here). 3) use the date when we update //build/util/LASTCHANGE or some other version file, which will get updated whenever we run hooks (possibly in conjunction w/ a build_date arg). 4) use the date of the most recent commit (the one embedded into LASTCHANGE), which wouldn't need a build_date arg to be reproducible. Thoughts?
What this CL is trying to do is this: We currently use __DATE__. That's bad because it makes our build nondeterministic. To fix, this makes the embedded date injectible from the outside. That's a decent approach imo -- it just needs to be injected as a literal, so that if a different time gets passed in, the build system can figure out that it needs to rebuild things.
zforman@google.com changed reviewers: - brettw@chromium.org
https://codereview.chromium.org/1641413002/diff/160001/base/build_time.cc File base/build_time.cc (right): https://codereview.chromium.org/1641413002/diff/160001/base/build_time.cc#new... base/build_time.cc:18: const char kDateTime[] = BUILD_DATE " 00:00:00"; On 2016/02/04 at 15:28:16, Nico wrote: > Can we make this 5am or something not right at the boundary? Have done - probably safer to do this. https://codereview.chromium.org/1641413002/diff/160001/base/build_time.h File base/build_time.h (right): https://codereview.chromium.org/1641413002/diff/160001/base/build_time.h#newc... base/build_time.h:17: // changes. However, official builds will always be built from scratch. On 2016/02/04 at 15:50:19, laforge wrote: > On 2016/02/04 15:24:13, M-A Ruel wrote: > > Technically, this is not 100% true. Adding Anthony so he can clarify this. > > That's true for our full release builds, however for our continuous official builds those do not get clobbered. Do you have any suggestion as to how you'd like continuous official builds to be handled?
zforman@google.com changed reviewers: + brettw@chromium.org
On 2016/02/04 at 22:39:25, Zachary Forman wrote: > zforman@google.com changed reviewers: > - brettw@chromium.org This was not intentional.
On 2016/02/04 at 22:35:31, thakis wrote: > What this CL is trying to do is this: > > We currently use __DATE__. That's bad because it makes our build nondeterministic. To fix, this makes the embedded date injectible from the outside. That's a decent approach imo -- it just needs to be injected as a literal, so that if a different time gets passed in, the build system can figure out that it needs to rebuild things. What we could do is use the various exec_script equivalents to get the desired BUILD_TIME define at gn/gyp time (so builds work properly) - I'd been avoiding this solely because it's not desirable to execute scripts from GN from a speed perspective - but it would be the better solution from most (all?) other angles.
This function is used for error classification and not actually detecting errors right? I wouldn't want to be throwing errors if somebody hasn't updated their build in a year.
On 2016/02/04 at 22:51:33, brettw wrote: > This function is used for error classification and not actually detecting errors right? I wouldn't want to be throwing errors if somebody hasn't updated their build in a year. Correct. See https://crbug.com/414843#c16
On 2016/02/04 22:43:54, Zachary Forman wrote: > On 2016/02/04 at 22:35:31, thakis wrote: > > What this CL is trying to do is this: > > > > We currently use __DATE__. That's bad because it makes our build > nondeterministic. To fix, this makes the embedded date injectible from the > outside. That's a decent approach imo -- it just needs to be injected as a > literal, so that if a different time gets passed in, the build system can figure > out that it needs to rebuild things. As noted, I'm not a fan of using __DATE__ (or any build date) in favor of getting a time from a server instead, but if that's not an option ... We agree that injecting (setting) a time at ninja (compile) time doesn't work because it would make the build dirty. My point is that we also can't inject a time at GN (generate_build_files) time, because ninja will re-invoke GN if it thinks it needs to anyway. So there's no real difference between during compile and during generate_build_files. I suggest we set the build date at runhooks time instead.
On 2016/02/04 22:56:15, Dirk Pranke wrote: > On 2016/02/04 22:43:54, Zachary Forman wrote: > > On 2016/02/04 at 22:35:31, thakis wrote: > > > What this CL is trying to do is this: > > > > > > We currently use __DATE__. That's bad because it makes our build > > nondeterministic. To fix, this makes the embedded date injectible from the > > outside. That's a decent approach imo -- it just needs to be injected as a > > literal, so that if a different time gets passed in, the build system can > figure > > out that it needs to rebuild things. > > As noted, I'm not a fan of using __DATE__ (or any build date) in favor of > getting > a time from a server instead, but if that's not an option ... > > We agree that injecting (setting) a time at ninja (compile) time doesn't work > because > it would make the build dirty. > > My point is that we also can't inject a time at GN (generate_build_files) time, > because > ninja will re-invoke GN if it thinks it needs to anyway. That's not a problem though if it's a literal date, right? If I pass build_time=2020/01/14, then the gn-reinvocation will get that date too. > So there's no real > difference > between during compile and during generate_build_files. > > I suggest we set the build date at runhooks time instead.
On 2016/02/04 22:54:06, lgarron wrote: > On 2016/02/04 at 22:51:33, brettw wrote: > > This function is used for error classification and not actually detecting > errors right? I wouldn't want to be throwing errors if somebody hasn't updated > their build in a year. > > Correct. See https://crbug.com/414843#c16 To make sure I'm understanding things: If 1) the cert has expired by the local clock's definition and 2) the cert has expired in comparison to the build date then we display the interstitial, right? So, if the build is old, as long as the client clock is ok, we don't care, right?
On 2016/02/04 23:00:09, Nico wrote: > On 2016/02/04 22:56:15, Dirk Pranke wrote: > > On 2016/02/04 22:43:54, Zachary Forman wrote: > > > On 2016/02/04 at 22:35:31, thakis wrote: > > > > What this CL is trying to do is this: > > > > > > > > We currently use __DATE__. That's bad because it makes our build > > > nondeterministic. To fix, this makes the embedded date injectible from the > > > outside. That's a decent approach imo -- it just needs to be injected as a > > > literal, so that if a different time gets passed in, the build system can > > figure > > > out that it needs to rebuild things. > > > > As noted, I'm not a fan of using __DATE__ (or any build date) in favor of > > getting > > a time from a server instead, but if that's not an option ... > > > > We agree that injecting (setting) a time at ninja (compile) time doesn't work > > because > > it would make the build dirty. > > > > My point is that we also can't inject a time at GN (generate_build_files) > time, > > because > > ninja will re-invoke GN if it thinks it needs to anyway. > > That's not a problem though if it's a literal date, right? If I pass > build_time=2020/01/14, then the gn-reinvocation will get that date too. That's correct, but who will be responsible for passing in build_time with a current value?
On 2016/02/04 at 23:01:31, dpranke wrote: > On 2016/02/04 23:00:09, Nico wrote: > > On 2016/02/04 22:56:15, Dirk Pranke wrote: > > > On 2016/02/04 22:43:54, Zachary Forman wrote: > > > > On 2016/02/04 at 22:35:31, thakis wrote: > > > > > What this CL is trying to do is this: > > > > > > > > > > We currently use __DATE__. That's bad because it makes our build > > > > nondeterministic. To fix, this makes the embedded date injectible from the > > > > outside. That's a decent approach imo -- it just needs to be injected as a > > > > literal, so that if a different time gets passed in, the build system can > > > figure > > > > out that it needs to rebuild things. > > > > > > As noted, I'm not a fan of using __DATE__ (or any build date) in favor of > > > getting > > > a time from a server instead, but if that's not an option ... > > > > > > We agree that injecting (setting) a time at ninja (compile) time doesn't work > > > because > > > it would make the build dirty. > > > > > > My point is that we also can't inject a time at GN (generate_build_files) > > time, > > > because > > > ninja will re-invoke GN if it thinks it needs to anyway. > > > > That's not a problem though if it's a literal date, right? If I pass > > build_time=2020/01/14, then the gn-reinvocation will get that date too. > > That's correct, but who will be responsible for passing in build_time with a current value? Could we not set it either via gn args, or if unset get a value with exec_script in gn? (equivalent variation for gyp). Then, to refresh the build date, just rerun gn gen / set a new arg?
On 2016/02/04 23:08:33, Zachary Forman wrote: > Could we not set it either via gn args, or if unset get a value with exec_script > in gn? (equivalent variation for gyp). > Then, to refresh the build date, just rerun gn gen / set a new arg? I push back pretty hard on exec script since it slows everything down, especially on Windows. More importantly, GN will re-run all the time when you update a build file. I don't want to be in a situation where every time I make a build change some file in base has to recompile. I think a sync hook like we do for VERSION would be most appropriate.
On 2016/02/04 23:17:47, brettw wrote: > On 2016/02/04 23:08:33, Zachary Forman wrote: > > Could we not set it either via gn args, or if unset get a value with > exec_script > > in gn? (equivalent variation for gyp). > > Then, to refresh the build date, just rerun gn gen / set a new arg? > > I push back pretty hard on exec script since it slows everything down, > especially on Windows. > > More importantly, GN will re-run all the time when you update a build file. I > don't want to be in a situation where every time I make a build change some file > in base has to recompile. > > I think a sync hook like we do for VERSION would be most appropriate. I'm not against using a runhook; basically we'd run this python script via DEPS / hooks. The problem is knowing if it is an official build or not. The fix is to make the header file have both: #ifndef BUILD_DATE #if OFFICIAL_BUILD #define BUILD_DATE <today> #else #define BUILD_DATE <first saturday of month> #endif #endif
On 2016/02/04 at 23:25:29, maruel wrote: > On 2016/02/04 23:17:47, brettw wrote: > > On 2016/02/04 23:08:33, Zachary Forman wrote: > > > Could we not set it either via gn args, or if unset get a value with > > exec_script > > > in gn? (equivalent variation for gyp). > > > Then, to refresh the build date, just rerun gn gen / set a new arg? > > > > I push back pretty hard on exec script since it slows everything down, > > especially on Windows. > > > > More importantly, GN will re-run all the time when you update a build file. I > > don't want to be in a situation where every time I make a build change some file > > in base has to recompile. > > > > I think a sync hook like we do for VERSION would be most appropriate. > > I'm not against using a runhook; basically we'd run this python script via DEPS / hooks. > > The problem is knowing if it is an official build or not. The fix is to make the header file have both: > #ifndef BUILD_DATE > #if OFFICIAL_BUILD > #define BUILD_DATE <today> > #else > #define BUILD_DATE <first saturday of month> > #endif > #endif That sounds reasonable to me. We can then have override_build_date simply add BUILD_DATE [[The argument]] to compile time flags.
I would be inclined to just use the same code path for both; I find it a bit hard to believe that this change itself will happen often than changing something else in base that would also cause you to rebuild and relink things, and it smells of premature optimization to me otherwise. I would also be inclined to use the date of LASTCHANGE as suggested earlier, unless there's some reason to think that wouldn't be a good idea?
On 2016/02/04 23:29:29, Zachary Forman wrote: > On 2016/02/04 at 23:25:29, maruel wrote: > > On 2016/02/04 23:17:47, brettw wrote: > > > On 2016/02/04 23:08:33, Zachary Forman wrote: > > > > Could we not set it either via gn args, or if unset get a value with > > > exec_script > > > > in gn? (equivalent variation for gyp). > > > > Then, to refresh the build date, just rerun gn gen / set a new arg? > > > > > > I push back pretty hard on exec script since it slows everything down, > > > especially on Windows. > > > > > > More importantly, GN will re-run all the time when you update a build file. > I > > > don't want to be in a situation where every time I make a build change some > file > > > in base has to recompile. > > > > > > I think a sync hook like we do for VERSION would be most appropriate. > > > > I'm not against using a runhook; basically we'd run this python script via > DEPS / hooks. > > > > The problem is knowing if it is an official build or not. The fix is to make > the header file have both: > > #ifndef BUILD_DATE > > #if OFFICIAL_BUILD > > #define BUILD_DATE <today> > > #else > > #define BUILD_DATE <first saturday of month> > > #endif > > #endif > > That sounds reasonable to me. We can then have override_build_date simply add > BUILD_DATE [[The argument]] to compile time flags. No, overriding would be done manually by the user by running the script manually. So you can remove all concept of override_build_date from GN and GYP files. On 2016/02/04 23:48:12, Dirk Pranke wrote: > I would also be inclined to use the date of LASTCHANGE as suggested earlier, > unless > there's some reason to think that wouldn't be a good idea? Changing the security properties of Chrome is out of scope of this change. The maximum variance we tolerate is [0, -23:59:59] for official builds.
On 2016/02/05 00:20:47, M-A Ruel wrote: > On 2016/02/04 23:48:12, Dirk Pranke wrote: > > I would also be inclined to use the date of LASTCHANGE as suggested earlier, > > unless > > there's some reason to think that wouldn't be a good idea? > > Changing the security properties of Chrome is out of scope of this change. The > maximum variance we tolerate is [0, -23:59:59] for official builds. Okay, but maybe we shouldn't land this change, then, and land a different change that does change Chrome to use the revision date rather than the build date?
On 2016/02/05 at 00:25:48, dpranke wrote: > On 2016/02/05 00:20:47, M-A Ruel wrote: > > On 2016/02/04 23:48:12, Dirk Pranke wrote: > > > I would also be inclined to use the date of LASTCHANGE as suggested earlier, > > > unless > > > there's some reason to think that wouldn't be a good idea? > > > > Changing the security properties of Chrome is out of scope of this change. The > > maximum variance we tolerate is [0, -23:59:59] for official builds. > > Okay, but maybe we shouldn't land this change, then, and land a different change > that does change Chrome to use the revision date rather than the build date? Using the revision date was the initial suggestion I had - agl pointed out (in an email discussion) that this risked a 70 day timeout on pinning elapsing too quickly.
On 2016/02/05 00:31:48, Zachary Forman wrote: > On 2016/02/05 at 00:25:48, dpranke wrote: > > Okay, but maybe we shouldn't land this change, then, and land a different > change > > that does change Chrome to use the revision date rather than the build date? > > Using the revision date was the initial suggestion I had - agl pointed out (in > an email discussion) that this risked a 70 day timeout on pinning elapsing too > quickly. We bump the revision on every released official build, so I'm not sure how revision date would be different from build date here?
On 2016/02/05 at 00:33:50, dpranke wrote: > On 2016/02/05 00:31:48, Zachary Forman wrote: > > On 2016/02/05 at 00:25:48, dpranke wrote: > > > Okay, but maybe we shouldn't land this change, then, and land a different > > change > > > that does change Chrome to use the revision date rather than the build date? > > > > Using the revision date was the initial suggestion I had - agl pointed out (in > > an email discussion) that this risked a 70 day timeout on pinning elapsing too > > quickly. > > We bump the revision on every released official build, so I'm not sure how revision > date would be different from build date here? I don't know - I'm not familiar enough with the context to make any claim confidently. I think the other concern with just taking the commit is maruel's point that it increases the number of cache misses. I'm fairly happy to go with whatever meets both maruel and the security team's needs best. I've also uploaded a new patchset that uses hooks as discussed above.
On 2016/02/05 02:20:50, Zachary Forman wrote: > I think the other concern with just taking the commit is maruel's point that it > increases the number of cache misses. Yeah, it might be better to use the timestamp of the last commit that changed //chrome/VERSION, which tends to happen once a day (which, again, is less frequently than //base is likely gonna change).
On 2016/02/05 at 02:57:59, dpranke wrote: > On 2016/02/05 02:20:50, Zachary Forman wrote: > > I think the other concern with just taking the commit is maruel's point that it > > increases the number of cache misses. > > Yeah, it might be better to use the timestamp of the last commit that changed > //chrome/VERSION, which tends to happen once a day (which, again, is less > frequently than //base is likely gonna change). My gripe with that is that //chrome/VERSION is not consistently updated daily or necessarily consistently. Here's a sample from the last 20: Feb 01 2016 20:03:13 GMT Jan 31 2016 20:03:30 GMT Jan 30 2016 20:03:07 GMT Jan 29 2016 20:03:14 GMT Jan 28 2016 20:03:43 GMT Jan 27 2016 20:03:16 GMT Jan 26 2016 20:03:41 GMT Jan 25 2016 20:03:03 GMT Jan 24 2016 20:03:08 GMT # - twice in same day Jan 24 2016 00:12:49 GMT # - skips 1 Jan 22 2016 08:02:30 GMT # - skips 1 Jan 20 2016 20:03:28 GMT Jan 19 2016 20:03:19 GMT Jan 19 2016 07:47:58 GMT # - skips 1 Jan 17 2016 18:08:00 GMT # - skips 1 Jan 15 2016 20:03:43 GMT Jan 14 2016 20:03:14 GMT Jan 13 2016 20:03:15 GMT Jan 12 2016 20:03:25 GMT Jan 11 2016 20:03:23 GMT
On 2016/02/05 03:06:12, Zachary Forman wrote: > On 2016/02/05 at 02:57:59, dpranke wrote: > > On 2016/02/05 02:20:50, Zachary Forman wrote: > > > I think the other concern with just taking the commit is maruel's point that > it > > > increases the number of cache misses. > > > > Yeah, it might be better to use the timestamp of the last commit that changed > > //chrome/VERSION, which tends to happen once a day (which, again, is less > > frequently than //base is likely gonna change). > > My gripe with that is that //chrome/VERSION is not consistently updated daily or > necessarily consistently. > Here's a sample from the last 20: > Feb 01 2016 20:03:13 GMT > Jan 31 2016 20:03:30 GMT > Jan 30 2016 20:03:07 GMT > Jan 29 2016 20:03:14 GMT > Jan 28 2016 20:03:43 GMT > Jan 27 2016 20:03:16 GMT > Jan 26 2016 20:03:41 GMT > Jan 25 2016 20:03:03 GMT > Jan 24 2016 20:03:08 GMT # - twice in same day > Jan 24 2016 00:12:49 GMT # - skips 1 > Jan 22 2016 08:02:30 GMT # - skips 1 > Jan 20 2016 20:03:28 GMT > Jan 19 2016 20:03:19 GMT > Jan 19 2016 07:47:58 GMT # - skips 1 > Jan 17 2016 18:08:00 GMT # - skips 1 > Jan 15 2016 20:03:43 GMT > Jan 14 2016 20:03:14 GMT > Jan 13 2016 20:03:15 GMT > Jan 12 2016 20:03:25 GMT > Jan 11 2016 20:03:23 GMT Is that a problem? It seems to update pretty consistently.
On 2016/02/05 15:48:49, Nico wrote: > Is that a problem? It seems to update pretty consistently. FWIU, Security people didn't want to depend on //chrome/VERSION. Maybe it's #closeenough, but it even becomes, it MUST be done diligently and properly stated. Another option: - Have GN touch a fake file or leverage a file already written to anyway, e.g. PRODUCT_DIR/build.ninja. - List this file as an input, so that the script is run every time GN was run. - Have the script NOT touch the file if the content match. I think we get the best of all worlds (GN still fast, incremental build works, null build work).
On 2016/02/05 18:37:37, M-A Ruel wrote: > On 2016/02/05 15:48:49, Nico wrote: > > Is that a problem? It seems to update pretty consistently. > > FWIU, Security people didn't want to depend on //chrome/VERSION. Maybe it's > #closeenough, but it even becomes, it MUST be done diligently and properly > stated. > > Another option: > - Have GN touch a fake file or leverage a file already written to anyway, e.g. > PRODUCT_DIR/build.ninja. > - List this file as an input, so that the script is run every time GN was run. > - Have the script NOT touch the file if the content match. > > I think we get the best of all worlds (GN still fast, incremental build works, > null build work). Yes, but it'd be nice of builds were deterministic by default. This way, you won't always get the same binary if you build at the same revision.
On 2016/02/05 18:39:10, Nico wrote: > On 2016/02/05 18:37:37, M-A Ruel wrote: > > On 2016/02/05 15:48:49, Nico wrote: > > > Is that a problem? It seems to update pretty consistently. > > > > FWIU, Security people didn't want to depend on //chrome/VERSION. Maybe it's > > #closeenough, but it even becomes, it MUST be done diligently and properly > > stated. > > > > Another option: > > - Have GN touch a fake file or leverage a file already written to anyway, e.g. > > PRODUCT_DIR/build.ninja. > > - List this file as an input, so that the script is run every time GN was run. > > - Have the script NOT touch the file if the content match. > > > > I think we get the best of all worlds (GN still fast, incremental build works, > > null build work). > > Yes, but it'd be nice of builds were deterministic by default. This way, you > won't always get the same binary if you build at the same revision. That's the trade off between two orthogonal goals. What we are settling on is that you build *is* deterministic for *one* month only. The non-deterministic part can be overridden by definition with a *single* knob. We're creating this single know.
On 2016/02/05 18:41:06, M-A Ruel wrote: > On 2016/02/05 18:39:10, Nico wrote: > > On 2016/02/05 18:37:37, M-A Ruel wrote: > > > On 2016/02/05 15:48:49, Nico wrote: > > > > Is that a problem? It seems to update pretty consistently. > > > > > > FWIU, Security people didn't want to depend on //chrome/VERSION. Maybe it's > > > #closeenough, but it even becomes, it MUST be done diligently and properly > > > stated. > > > > > > Another option: > > > - Have GN touch a fake file or leverage a file already written to anyway, > e.g. > > > PRODUCT_DIR/build.ninja. > > > - List this file as an input, so that the script is run every time GN was > run. > > > - Have the script NOT touch the file if the content match. > > > > > > I think we get the best of all worlds (GN still fast, incremental build > works, > > > null build work). > > > > Yes, but it'd be nice of builds were deterministic by default. This way, you > > won't always get the same binary if you build at the same revision. > > That's the trade off between two orthogonal goals. What we are settling on is > that you build *is* deterministic for *one* month only. The non-deterministic > part can be overridden by definition with a *single* knob. We're creating this > single know. "single knob" in this CL.
Zero knobs would be nicer is all I'm saying :-) Can you forward me the discussions with security people?
On 2016/02/05 18:43:15, Nico wrote: > Zero knobs would be nicer is all I'm saying :-) > > Can you forward me the discussions with security people? Adrienne, Adam and Lucas are already cc'ed on this CL. They are the relevant people. (I should have come say hi, I was in MTV this week but I'm already at the airport now)
On 2016/02/05 18:53:50, M-A Ruel wrote: > On 2016/02/05 18:43:15, Nico wrote: > > Zero knobs would be nicer is all I'm saying :-) > > > > Can you forward me the discussions with security people? > > Adrienne, Adam and Lucas are already cc'ed on this CL. They are the relevant > people. (I should have come say hi, I was in MTV this week but I'm already at > the airport now) I'm also in favor of the "zero knobs" solution, and I'm not seeing any reason that <date of last change to //chrome/VERSION> is less suitable (or secure) than __DATE__ of build, but it would be good to get security approval.
On 2016/02/05 18:59:50, Dirk Pranke wrote: > On 2016/02/05 18:53:50, M-A Ruel wrote: > > On 2016/02/05 18:43:15, Nico wrote: > > > Zero knobs would be nicer is all I'm saying :-) > > > > > > Can you forward me the discussions with security people? > > > > Adrienne, Adam and Lucas are already cc'ed on this CL. They are the relevant > > people. (I should have come say hi, I was in MTV this week but I'm already at > > the airport now) > > I'm also in favor of the "zero knobs" solution, and I'm not seeing any reason > that > <date of last change to //chrome/VERSION> is less suitable (or secure) than > __DATE__ of build, but it would be good to get security approval. I don't think there's any difference in using //chrome/VERSION or using PRODUCT_DIR/build.ninja that makes //chrome/VERSION more interesting.
On 2016/02/05 19:10:23, M-A Ruel wrote: > On 2016/02/05 18:59:50, Dirk Pranke wrote: > > On 2016/02/05 18:53:50, M-A Ruel wrote: > > > On 2016/02/05 18:43:15, Nico wrote: > > > > Zero knobs would be nicer is all I'm saying :-) > > > > > > > > Can you forward me the discussions with security people? > > > > > > Adrienne, Adam and Lucas are already cc'ed on this CL. They are the relevant > > > people. (I should have come say hi, I was in MTV this week but I'm already > at > > > the airport now) > > > > I'm also in favor of the "zero knobs" solution, and I'm not seeing any reason > > that > > <date of last change to //chrome/VERSION> is less suitable (or secure) than > > __DATE__ of build, but it would be good to get security approval. > > I don't think there's any difference in using //chrome/VERSION or using > PRODUCT_DIR/build.ninja that makes //chrome/VERSION more interesting. Sure: build.ninja is written every time you run gyp or gn. chrome/VERSION isn't.
But I think patchset #3 is fine as a hook, no need for something more sophisticated. https://codereview.chromium.org/1641413002/diff/200001/build/write_build_date... File build/write_build_date_header.py (right): https://codereview.chromium.org/1641413002/diff/200001/build/write_build_date... build/write_build_date_header.py:58: with open(args.output_file, 'w') as output_file: Change to: - construct the content - read the file if it existed - if expected_content != actual_content: - write file this fixes incremental and null builds.
On 2016/02/05 19:10:23, M-A Ruel wrote: > On 2016/02/05 18:59:50, Dirk Pranke wrote: > > I'm also in favor of the "zero knobs" solution, and I'm not seeing any reason > > that > > <date of last change to //chrome/VERSION> is less suitable (or secure) than > > __DATE__ of build, but it would be good to get security approval. > > I don't think there's any difference in using //chrome/VERSION or using > PRODUCT_DIR/build.ninja that makes //chrome/VERSION more interesting. Maybe I'm not being clear. I'm not talking about the timestamp of the file in the local filesystem, I'm talking about the actual date of the commit itself (git metadata). That has the advantage that it is completely deterministic for a checkout at a single git revision, so you don't need any build arguments to override or reproduce things.
On 2016/02/05 19:39:08, Dirk Pranke wrote: > On 2016/02/05 19:10:23, M-A Ruel wrote: > > On 2016/02/05 18:59:50, Dirk Pranke wrote: > > > I'm also in favor of the "zero knobs" solution, and I'm not seeing any > reason > > > that > > > <date of last change to //chrome/VERSION> is less suitable (or secure) than > > > __DATE__ of build, but it would be good to get security approval. > > > > I don't think there's any difference in using //chrome/VERSION or using > > PRODUCT_DIR/build.ninja that makes //chrome/VERSION more interesting. > > Maybe I'm not being clear. I'm not talking about the timestamp of the file in > the local > filesystem, I'm talking about the actual date of the commit itself (git > metadata). > > That has the advantage that it is completely deterministic for a checkout at a > single > git revision, so you don't need any build arguments to override or reproduce > things. That's true but you will need to get the sign off from sec folks. Since Zachary is leaving next week and is based in Sydney, I prefer no fundamental security change in this CL so we can get immediate benefit without delays.
On 2016/02/05 at 19:43:54, maruel wrote: > On 2016/02/05 19:39:08, Dirk Pranke wrote: > > On 2016/02/05 19:10:23, M-A Ruel wrote: > > > On 2016/02/05 18:59:50, Dirk Pranke wrote: > > > > I'm also in favor of the "zero knobs" solution, and I'm not seeing any > > reason > > > > that > > > > <date of last change to //chrome/VERSION> is less suitable (or secure) than > > > > __DATE__ of build, but it would be good to get security approval. > > > > > > I don't think there's any difference in using //chrome/VERSION or using > > > PRODUCT_DIR/build.ninja that makes //chrome/VERSION more interesting. > > > > Maybe I'm not being clear. I'm not talking about the timestamp of the file in > > the local > > filesystem, I'm talking about the actual date of the commit itself (git > > metadata). > > > > That has the advantage that it is completely deterministic for a checkout at a > > single > > git revision, so you don't need any build arguments to override or reproduce > > things. > > That's true but you will need to get the sign off from sec folks. Since Zachary is leaving next week and is based in Sydney, I prefer no fundamental security change in this CL so we can get immediate benefit without delays. I agree. Something to consider: People branch Chrome for local development and sometimes to make their own browser. Maybe some of them don't change the last commit/VERSION date. If it's not too onerous, we should fail safe by keeping the protections that rely a fresh build date.
On 2016/02/05 19:58:00, lgarron wrote: > On 2016/02/05 at 19:43:54, maruel wrote: > > On 2016/02/05 19:39:08, Dirk Pranke wrote: > > > On 2016/02/05 19:10:23, M-A Ruel wrote: > > > > On 2016/02/05 18:59:50, Dirk Pranke wrote: > > > > > I'm also in favor of the "zero knobs" solution, and I'm not seeing any > > > reason > > > > > that > > > > > <date of last change to //chrome/VERSION> is less suitable (or secure) > than > > > > > __DATE__ of build, but it would be good to get security approval. > > > > > > > > I don't think there's any difference in using //chrome/VERSION or using > > > > PRODUCT_DIR/build.ninja that makes //chrome/VERSION more interesting. > > > > > > Maybe I'm not being clear. I'm not talking about the timestamp of the file > in > > > the local > > > filesystem, I'm talking about the actual date of the commit itself (git > > > metadata). > > > > > > That has the advantage that it is completely deterministic for a checkout at > a > > > single > > > git revision, so you don't need any build arguments to override or reproduce > > > things. > > > > That's true but you will need to get the sign off from sec folks. Since > Zachary is leaving next week and is based in Sydney, I prefer no fundamental > security change in this CL so we can get immediate benefit without delays. > > I agree. > Something to consider: People branch Chrome for local development and sometimes > to make their own browser. Maybe some of them don't change the last > commit/VERSION date. If it's not too onerous, we should fail safe by keeping the > protections that rely a fresh build date. We could take the date of the HEAD commit. Locally-branched builds will have this at a recent revision. I don't think we need to rush this change in if we think it's not the right long-term approach. Relying on a date stamped into the binary for security-related things seems like somewhat brittle in the first place, and getting the date off a recent commit doesn't seem worse than whatever __DATE__ expands to. (While having deterministic builds has all kinds of benefits, including for security, as it allows dwheeler's mitigation against trusting trust attacks for example) What's the qualitative difference between __DATE__ and the date of a recent commit from a security PoV?
On 2016/02/05 at 20:03:37, thakis wrote: > On 2016/02/05 19:58:00, lgarron wrote: > > On 2016/02/05 at 19:43:54, maruel wrote: > > > On 2016/02/05 19:39:08, Dirk Pranke wrote: > > > > On 2016/02/05 19:10:23, M-A Ruel wrote: > > > > > On 2016/02/05 18:59:50, Dirk Pranke wrote: > > > > > > I'm also in favor of the "zero knobs" solution, and I'm not seeing any > > > > reason > > > > > > that > > > > > > <date of last change to //chrome/VERSION> is less suitable (or secure) > > than > > > > > > __DATE__ of build, but it would be good to get security approval. > > > > > > > > > > I don't think there's any difference in using //chrome/VERSION or using > > > > > PRODUCT_DIR/build.ninja that makes //chrome/VERSION more interesting. > > > > > > > > Maybe I'm not being clear. I'm not talking about the timestamp of the file > > in > > > > the local > > > > filesystem, I'm talking about the actual date of the commit itself (git > > > > metadata). > > > > > > > > That has the advantage that it is completely deterministic for a checkout at > > a > > > > single > > > > git revision, so you don't need any build arguments to override or reproduce > > > > things. > > > > > > That's true but you will need to get the sign off from sec folks. Since > > Zachary is leaving next week and is based in Sydney, I prefer no fundamental > > security change in this CL so we can get immediate benefit without delays. > > > > I agree. > > Something to consider: People branch Chrome for local development and sometimes > > to make their own browser. Maybe some of them don't change the last > > commit/VERSION date. If it's not too onerous, we should fail safe by keeping the > > protections that rely a fresh build date. > > We could take the date of the HEAD commit. Locally-branched builds will have this at a recent revision. > > I don't think we need to rush this change in if we think it's not the right long-term approach. > > Relying on a date stamped into the binary for security-related things seems like somewhat brittle in the first place, and getting the date off a recent commit doesn't seem worse than whatever __DATE__ expands to. (While having deterministic builds has all kinds of benefits, including for security, as it allows dwheeler's mitigation against trusting trust attacks for example) What's the qualitative difference between __DATE__ and the date of a recent commit from a security PoV? You lose all preloaded HSTS and key pinning: https://code.google.com/p/chromium/codesearch#chromium/src/net/http/transport...
On 2016/02/05 20:09:09, lgarron wrote: > On 2016/02/05 at 20:03:37, thakis wrote: > > On 2016/02/05 19:58:00, lgarron wrote: > > > On 2016/02/05 at 19:43:54, maruel wrote: > > > > On 2016/02/05 19:39:08, Dirk Pranke wrote: > > > > > On 2016/02/05 19:10:23, M-A Ruel wrote: > > > > > > On 2016/02/05 18:59:50, Dirk Pranke wrote: > > > > > > > I'm also in favor of the "zero knobs" solution, and I'm not seeing > any > > > > > reason > > > > > > > that > > > > > > > <date of last change to //chrome/VERSION> is less suitable (or > secure) > > > than > > > > > > > __DATE__ of build, but it would be good to get security approval. > > > > > > > > > > > > I don't think there's any difference in using //chrome/VERSION or > using > > > > > > PRODUCT_DIR/build.ninja that makes //chrome/VERSION more interesting. > > > > > > > > > > Maybe I'm not being clear. I'm not talking about the timestamp of the > file > > > in > > > > > the local > > > > > filesystem, I'm talking about the actual date of the commit itself (git > > > > > metadata). > > > > > > > > > > That has the advantage that it is completely deterministic for a > checkout at > > > a > > > > > single > > > > > git revision, so you don't need any build arguments to override or > reproduce > > > > > things. > > > > > > > > That's true but you will need to get the sign off from sec folks. Since > > > Zachary is leaving next week and is based in Sydney, I prefer no fundamental > > > security change in this CL so we can get immediate benefit without delays. > > > > > > I agree. > > > Something to consider: People branch Chrome for local development and > sometimes > > > to make their own browser. Maybe some of them don't change the last > > > commit/VERSION date. If it's not too onerous, we should fail safe by keeping > the > > > protections that rely a fresh build date. > > > > We could take the date of the HEAD commit. Locally-branched builds will have > this at a recent revision. > > > > I don't think we need to rush this change in if we think it's not the right > long-term approach. > > > > Relying on a date stamped into the binary for security-related things seems > like somewhat brittle in the first place, and getting the date off a recent > commit doesn't seem worse than whatever __DATE__ expands to. (While having > deterministic builds has all kinds of benefits, including for security, as it > allows dwheeler's mitigation against trusting trust attacks for example) What's > the qualitative difference between __DATE__ and the date of a recent commit from > a security PoV? > > You lose all preloaded HSTS and key pinning: > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/transport... __DATE__ and "recent commit" both will evaluate to roughly "today" (unless my local branch doesn't have any commits newer than N months), so I don't understand why this would be lost.
On 2016/02/05 at 20:11:02, thakis wrote: > On 2016/02/05 20:09:09, lgarron wrote: > > On 2016/02/05 at 20:03:37, thakis wrote: > > > On 2016/02/05 19:58:00, lgarron wrote: > > > > On 2016/02/05 at 19:43:54, maruel wrote: > > > > > On 2016/02/05 19:39:08, Dirk Pranke wrote: > > > > > > On 2016/02/05 19:10:23, M-A Ruel wrote: > > > > > > > On 2016/02/05 18:59:50, Dirk Pranke wrote: > > > > > > > > I'm also in favor of the "zero knobs" solution, and I'm not seeing > > any > > > > > > reason > > > > > > > > that > > > > > > > > <date of last change to //chrome/VERSION> is less suitable (or > > secure) > > > > than > > > > > > > > __DATE__ of build, but it would be good to get security approval. > > > > > > > > > > > > > > I don't think there's any difference in using //chrome/VERSION or > > using > > > > > > > PRODUCT_DIR/build.ninja that makes //chrome/VERSION more interesting. > > > > > > > > > > > > Maybe I'm not being clear. I'm not talking about the timestamp of the > > file > > > > in > > > > > > the local > > > > > > filesystem, I'm talking about the actual date of the commit itself (git > > > > > > metadata). > > > > > > > > > > > > That has the advantage that it is completely deterministic for a > > checkout at > > > > a > > > > > > single > > > > > > git revision, so you don't need any build arguments to override or > > reproduce > > > > > > things. > > > > > > > > > > That's true but you will need to get the sign off from sec folks. Since > > > > Zachary is leaving next week and is based in Sydney, I prefer no fundamental > > > > security change in this CL so we can get immediate benefit without delays. > > > > > > > > I agree. > > > > Something to consider: People branch Chrome for local development and > > sometimes > > > > to make their own browser. Maybe some of them don't change the last > > > > commit/VERSION date. If it's not too onerous, we should fail safe by keeping > > the > > > > protections that rely a fresh build date. > > > > > > We could take the date of the HEAD commit. Locally-branched builds will have > > this at a recent revision. > > > > > > I don't think we need to rush this change in if we think it's not the right > > long-term approach. > > > > > > Relying on a date stamped into the binary for security-related things seems > > like somewhat brittle in the first place, and getting the date off a recent > > commit doesn't seem worse than whatever __DATE__ expands to. (While having > > deterministic builds has all kinds of benefits, including for security, as it > > allows dwheeler's mitigation against trusting trust attacks for example) What's > > the qualitative difference between __DATE__ and the date of a recent commit from > > a security PoV? > > > > You lose all preloaded HSTS and key pinning: > > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/transport... > > __DATE__ and "recent commit" both will evaluate to roughly "today" (unless my local branch doesn't have any commits newer than N months), so I don't understand why this would be lost. For Chrome devs, you're right, they will usually be working in git on a branch that is usually at most a few weeks old.* But I'd *prefer* not to assume that this is the case for everyone everyone who builds from the Chrome source tree. * I tend to rebase my branches into a single commit each, so the HEAD commit date for my branch tends to stay what it was soon after I branched (even if I rebase onto a newer branch – my branch commit dates are not monotonic!). It might sometimes be useful to switch to a branch that hasn't had any commit changes in a long time.
On 2016/02/05 20:23:24, lgarron wrote: > On 2016/02/05 at 20:11:02, thakis wrote: > > On 2016/02/05 20:09:09, lgarron wrote: > > > On 2016/02/05 at 20:03:37, thakis wrote: > > > > On 2016/02/05 19:58:00, lgarron wrote: > > > > > On 2016/02/05 at 19:43:54, maruel wrote: > > > > > > On 2016/02/05 19:39:08, Dirk Pranke wrote: > > > > > > > On 2016/02/05 19:10:23, M-A Ruel wrote: > > > > > > > > On 2016/02/05 18:59:50, Dirk Pranke wrote: > > > > > > > > > I'm also in favor of the "zero knobs" solution, and I'm not > seeing > > > any > > > > > > > reason > > > > > > > > > that > > > > > > > > > <date of last change to //chrome/VERSION> is less suitable (or > > > secure) > > > > > than > > > > > > > > > __DATE__ of build, but it would be good to get security > approval. > > > > > > > > > > > > > > > > I don't think there's any difference in using //chrome/VERSION or > > > using > > > > > > > > PRODUCT_DIR/build.ninja that makes //chrome/VERSION more > interesting. > > > > > > > > > > > > > > Maybe I'm not being clear. I'm not talking about the timestamp of > the > > > file > > > > > in > > > > > > > the local > > > > > > > filesystem, I'm talking about the actual date of the commit itself > (git > > > > > > > metadata). > > > > > > > > > > > > > > That has the advantage that it is completely deterministic for a > > > checkout at > > > > > a > > > > > > > single > > > > > > > git revision, so you don't need any build arguments to override or > > > reproduce > > > > > > > things. > > > > > > > > > > > > That's true but you will need to get the sign off from sec folks. > Since > > > > > Zachary is leaving next week and is based in Sydney, I prefer no > fundamental > > > > > security change in this CL so we can get immediate benefit without > delays. > > > > > > > > > > I agree. > > > > > Something to consider: People branch Chrome for local development and > > > sometimes > > > > > to make their own browser. Maybe some of them don't change the last > > > > > commit/VERSION date. If it's not too onerous, we should fail safe by > keeping > > > the > > > > > protections that rely a fresh build date. > > > > > > > > We could take the date of the HEAD commit. Locally-branched builds will > have > > > this at a recent revision. > > > > > > > > I don't think we need to rush this change in if we think it's not the > right > > > long-term approach. > > > > > > > > Relying on a date stamped into the binary for security-related things > seems > > > like somewhat brittle in the first place, and getting the date off a recent > > > commit doesn't seem worse than whatever __DATE__ expands to. (While having > > > deterministic builds has all kinds of benefits, including for security, as > it > > > allows dwheeler's mitigation against trusting trust attacks for example) > What's > > > the qualitative difference between __DATE__ and the date of a recent commit > from > > > a security PoV? > > > > > > You lose all preloaded HSTS and key pinning: > > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/transport... > > > > __DATE__ and "recent commit" both will evaluate to roughly "today" (unless my > local branch doesn't have any commits newer than N months), so I don't > understand why this would be lost. > > For Chrome devs, you're right, they will usually be working in git on a branch > that is usually at most a few weeks old.* > But I'd *prefer* not to assume that this is the case for everyone everyone who > builds from the Chrome source tree. It is fair to try and design a solution that works for a wide range of scenarios, but I would prefer not to have to settle for something that is less than ideal in order to do so. I think, at least by default, we should have a solution that works for Chrome and other vendors that are hopefully following sane build and release processes (i.e., incrementing versions or build numbers on every released build) and preserves the properties we want: determinism and reproducibility with a minimal amount of configuration overhead by default. I never did see an answer to my original observation and question, though, which is that using a client side build date is silly if we can query the server instead. Is that not possible?
On 2016/02/05 at 20:36:13, dpranke wrote: > On 2016/02/05 20:23:24, lgarron wrote: > > On 2016/02/05 at 20:11:02, thakis wrote: > > > On 2016/02/05 20:09:09, lgarron wrote: > > > > On 2016/02/05 at 20:03:37, thakis wrote: > > > > > On 2016/02/05 19:58:00, lgarron wrote: > > > > > > On 2016/02/05 at 19:43:54, maruel wrote: > > > > > > > On 2016/02/05 19:39:08, Dirk Pranke wrote: > > > > > > > > On 2016/02/05 19:10:23, M-A Ruel wrote: > > > > > > > > > On 2016/02/05 18:59:50, Dirk Pranke wrote: > > > > > > > > > > I'm also in favor of the "zero knobs" solution, and I'm not > > seeing > > > > any > > > > > > > > reason > > > > > > > > > > that > > > > > > > > > > <date of last change to //chrome/VERSION> is less suitable (or > > > > secure) > > > > > > than > > > > > > > > > > __DATE__ of build, but it would be good to get security > > approval. > > > > > > > > > > > > > > > > > > I don't think there's any difference in using //chrome/VERSION or > > > > using > > > > > > > > > PRODUCT_DIR/build.ninja that makes //chrome/VERSION more > > interesting. > > > > > > > > > > > > > > > > Maybe I'm not being clear. I'm not talking about the timestamp of > > the > > > > file > > > > > > in > > > > > > > > the local > > > > > > > > filesystem, I'm talking about the actual date of the commit itself > > (git > > > > > > > > metadata). > > > > > > > > > > > > > > > > That has the advantage that it is completely deterministic for a > > > > checkout at > > > > > > a > > > > > > > > single > > > > > > > > git revision, so you don't need any build arguments to override or > > > > reproduce > > > > > > > > things. > > > > > > > > > > > > > > That's true but you will need to get the sign off from sec folks. > > Since > > > > > > Zachary is leaving next week and is based in Sydney, I prefer no > > fundamental > > > > > > security change in this CL so we can get immediate benefit without > > delays. > > > > > > > > > > > > I agree. > > > > > > Something to consider: People branch Chrome for local development and > > > > sometimes > > > > > > to make their own browser. Maybe some of them don't change the last > > > > > > commit/VERSION date. If it's not too onerous, we should fail safe by > > keeping > > > > the > > > > > > protections that rely a fresh build date. > > > > > > > > > > We could take the date of the HEAD commit. Locally-branched builds will > > have > > > > this at a recent revision. > > > > > > > > > > I don't think we need to rush this change in if we think it's not the > > right > > > > long-term approach. > > > > > > > > > > Relying on a date stamped into the binary for security-related things > > seems > > > > like somewhat brittle in the first place, and getting the date off a recent > > > > commit doesn't seem worse than whatever __DATE__ expands to. (While having > > > > deterministic builds has all kinds of benefits, including for security, as > > it > > > > allows dwheeler's mitigation against trusting trust attacks for example) > > What's > > > > the qualitative difference between __DATE__ and the date of a recent commit > > from > > > > a security PoV? > > > > > > > > You lose all preloaded HSTS and key pinning: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/transport... > > > > > > __DATE__ and "recent commit" both will evaluate to roughly "today" (unless my > > local branch doesn't have any commits newer than N months), so I don't > > understand why this would be lost. > > > > For Chrome devs, you're right, they will usually be working in git on a branch > > that is usually at most a few weeks old.* > > But I'd *prefer* not to assume that this is the case for everyone everyone who > > builds from the Chrome source tree. > > It is fair to try and design a solution that works for a wide range of scenarios, but > I would prefer not to have to settle for something that is less than ideal in order > to do so. I think, at least by default, we should have a solution that works for > Chrome and other vendors that are hopefully following sane build and release > processes (i.e., incrementing versions or build numbers on every released build) > and preserves the properties we want: determinism and reproducibility with a > minimal amount of configuration overhead by default. > > I never did see an answer to my original observation and question, though, which > is that using a client side build date is silly if we can query the server instead. > Is that not possible? Trusted time from a server is possible, and we are strongly considering using it (mostly because *a lot* of client clocks are off) But we're not there yet.
> It is fair to try and design a solution that works for a wide range of > scenarios, but > I would prefer not to have to settle for something that is less than ideal in > order > to do so. I think, at least by default, we should have a solution that works for > > Chrome and other vendors that are hopefully following sane build and release > processes (i.e., incrementing versions or build numbers on every released build) > and preserves the properties we want: determinism and reproducibility with a > minimal amount of configuration overhead by default. I strongly agree with this.
> I never did see an answer to my original observation and question, though, which > is that using a client side build date is silly if we can query the server > instead. > Is that not possible? Having a build contact the outside world is even worse IMHO; I wouldn't be able to compile offline. Many devs (I forget if it is Intel or Samsung?) have non-transparent HTTP proxy on their workstation. On 2016/02/05 20:03:37, Nico wrote: > I don't think we need to rush this change in if we think it's not the right > long-term approach. Are you implying you will take up Zachary's change when he leaves in 6 days? Because I don't have time available to take this work. And not committing this leaves us in a *worse* situation. I don't understand the push back here. Filed https://code.google.com/p/chromium/issues/detail?id=584880 if you really feel the urge to take on this. https://codereview.chromium.org/1641413002/diff/200001/.gitignore File .gitignore (right): https://codereview.chromium.org/1641413002/diff/200001/.gitignore#newcode62 .gitignore:62: /base/generated_build_date.h I was expecting this file to live in PRODUCT_DIR/(generated_files_something>/base/generated_build_date.h No need to update .gitignore. https://codereview.chromium.org/1641413002/diff/200001/DEPS File DEPS (right): https://codereview.chromium.org/1641413002/diff/200001/DEPS#newcode636 DEPS:636: 'src/base/generated_build_date.h'], The path should be inside PRODUCT_DIR. https://codereview.chromium.org/1641413002/diff/200001/third_party/yasm/CHROM... File third_party/yasm/CHROMIUM.diff (right): https://codereview.chromium.org/1641413002/diff/200001/third_party/yasm/CHROM... third_party/yasm/CHROMIUM.diff:7: - "Compiled on " __DATE__ ".", We actually need to update the code. So skip the change to this file, I'll take a stab since it requires a DEPS roll. https://code.google.com/p/chromium/issues/detail?id=584876
On 2016/02/06 02:24:37, M-A Ruel wrote: > > I never did see an answer to my original observation and question, though, > which > > is that using a client side build date is silly if we can query the server > > instead. > > Is that not possible? > > Having a build contact the outside world is even worse IMHO; I wouldn't be able > to compile offline. Many devs (I forget if it is Intel or Samsung?) have > non-transparent HTTP proxy on their workstation. We wouldn't contact a server at compile time, we'd contact a server at runtime. > And not committing this leaves us in a *worse* situation. > I don't understand the push back here. In my view the current code isn't good, but the change doesn't make things all that much better (by changing the file from updating once a day to once a month). As I've said earlier in the thread, //base tends to change multiple times a day, so I'd be surprised if this change allows us to reuse significantly more builds, and it makes the overall system more complex in a way that isn't particularly clear. I think changing the code to look at the timestamp of the last commit to //chrome/VERSION, on the other hand, is probably a simpler patch and at least produces something that is fairly clear while making the build reproducible for *any* kind of build (official or not, dont_embed setting or not), at least as far as this file goes.
On 2016/02/06 17:39:01, Dirk Pranke wrote: > > And not committing this leaves us in a *worse* situation. > > I don't understand the push back here. > > In my view the current code isn't good, but the change doesn't make things all > that much better (by changing the file from updating once a day to once a > month). You seriously misunderstand this change. Reread: https://codereview.chromium.org/1641413002/diff/200001/base/build_time.cc It changes from "never enforcing EV compliance nor HSTS pinning on chromium builds on CI" to "enforcing it for 40 days". In addition, this removes the need for define DONT_EMBED_BUILD_METADATA and for dont_embed_build_metadata. We found recipes were people had forgot to set this variable. This is a net win. Which reminds me, Zachary you want to remove references to dont_embed_build_metadata. > As I've said earlier in the thread, //base tends to change multiple > times > a day, so I'd be surprised if this change allows us to reuse significantly more > builds, > and it makes the overall system more complex in a way that isn't particularly > clear. This is irrelevant. We accept to invalidate builds once a month as a security trade off to better test chromium builds. > I think changing the code to look at the timestamp of the last commit to > //chrome/VERSION, on the other hand, is probably a simpler patch and at least > produces something that is fairly clear while making the build reproducible > for *any* kind of build (official or not, dont_embed setting or not), at least > as far as this file goes. I clearly stated changing the security properties of the official build is out of scope of this CL. If you want to advocate for this change, please make a CL referencing https://code.google.com/p/chromium/issues/detail?id=584880. Zachary is leaving in 5 days, I have 16 hours difference with him and he has other things to close too, so this CL has only ~3 back and forth before he is done with it on Wednesday his time, which is Tuesday for me.
Alright, so based on the discussion I've made a new patchset based on the gn/gyp + ninja solution. It adds //build/util/LASTCHANGE as a dependency of the generator script, but will only modify the generated file if the generated file differs from what the output should be. I've removed most instances where the dont_embed_build_metadata flag is used, but haven't removed it completely (since it would kill bots that use it). https://codereview.chromium.org/1641413002/diff/200001/build/write_build_date... File build/write_build_date_header.py (right): https://codereview.chromium.org/1641413002/diff/200001/build/write_build_date... build/write_build_date_header.py:58: with open(args.output_file, 'w') as output_file: On 2016/02/05 at 19:16:37, M-A Ruel wrote: > Change to: > > - construct the content > - read the file if it existed > - if expected_content != actual_content: > - write file > > this fixes incremental and null builds. Done https://codereview.chromium.org/1641413002/diff/200001/third_party/yasm/CHROM... File third_party/yasm/CHROMIUM.diff (right): https://codereview.chromium.org/1641413002/diff/200001/third_party/yasm/CHROM... third_party/yasm/CHROMIUM.diff:7: - "Compiled on " __DATE__ ".", On 2016/02/06 at 02:24:37, M-A Ruel wrote: > We actually need to update the code. So skip the change to this file, I'll take a stab since it requires a DEPS roll. https://code.google.com/p/chromium/issues/detail?id=584876 Ack
looks great, few nits. https://codereview.chromium.org/1641413002/diff/240001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1641413002/diff/240001/base/BUILD.gn#newcode1894 base/BUILD.gn:1894: "//build/util/LASTCHANGE", This is even better. https://codereview.chromium.org/1641413002/diff/240001/base/BUILD.gn#newcode1901 base/BUILD.gn:1901: [ rebase_path("$target_gen_dir/generated_build_date.h", root_build_dir) ] That's what gn format did? That's surprising. https://codereview.chromium.org/1641413002/diff/240001/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/1641413002/diff/240001/base/base.gyp#newcode1008 base/base.gyp:1008: '<(SHARED_INTERMEDIATE_DIR)/base/generated_build_date.h', '<(build_type)' wrap line https://codereview.chromium.org/1641413002/diff/240001/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/1641413002/diff/240001/base/base.gypi#newcode774 base/base.gypi:774: '<@(trace_event_sources)' Skip this file from this change. https://codereview.chromium.org/1641413002/diff/240001/base/build_time.cc File base/build_time.cc (right): https://codereview.chromium.org/1641413002/diff/240001/base/build_time.cc#new... base/build_time.cc:18: const char kDateTime[] = BUILD_DATE " 05:00:00"; Why 5? https://codereview.chromium.org/1641413002/diff/240001/base/build_time.h File base/build_time.h (right): https://codereview.chromium.org/1641413002/diff/240001/base/build_time.h#newc... base/build_time.h:21: // It will always be in the past. It is guaranteed to be in the past in UTC.
https://codereview.chromium.org/1641413002/diff/240001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1641413002/diff/240001/base/BUILD.gn#newcode1894 base/BUILD.gn:1894: "//build/util/LASTCHANGE", On 2016/02/09 at 00:14:13, M-A Ruel wrote: > This is even better. Still requires running hooks, which is perhaps suboptimal - but it makes the most sense. https://codereview.chromium.org/1641413002/diff/240001/base/BUILD.gn#newcode1901 base/BUILD.gn:1901: [ rebase_path("$target_gen_dir/generated_build_date.h", root_build_dir) ] On 2016/02/09 at 00:14:13, M-A Ruel wrote: > That's what gn format did? That's surprising. Surprisingly, yes: $ gn format base/BUILD.gn | sed -n '1901p' [ rebase_path("$target_gen_dir/generated_build_date.h", root_build_dir) ] https://codereview.chromium.org/1641413002/diff/240001/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/1641413002/diff/240001/base/base.gyp#newcode1008 base/base.gyp:1008: '<(SHARED_INTERMEDIATE_DIR)/base/generated_build_date.h', '<(build_type)' On 2016/02/09 at 00:14:13, M-A Ruel wrote: > wrap line Done https://codereview.chromium.org/1641413002/diff/240001/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/1641413002/diff/240001/base/base.gypi#newcode774 base/base.gypi:774: '<@(trace_event_sources)' On 2016/02/09 at 00:14:13, M-A Ruel wrote: > Skip this file from this change. Oops - corrected. https://codereview.chromium.org/1641413002/diff/240001/base/build_time.cc File base/build_time.cc (right): https://codereview.chromium.org/1641413002/diff/240001/base/build_time.cc#new... base/build_time.cc:18: const char kDateTime[] = BUILD_DATE " 05:00:00"; On 2016/02/09 at 00:14:13, M-A Ruel wrote: > Why 5? Arbitrary decision based on Thakis' suggestion of > base/build_time.cc:18: const char kDateTime[] = BUILD_DATE " 00:00:00"; > Can we make this 5am or something not right at the boundary? I think it makes sense to use something not midnight (so we don't get caught by some weird edge case). https://codereview.chromium.org/1641413002/diff/240001/base/build_time.h File base/build_time.h (right): https://codereview.chromium.org/1641413002/diff/240001/base/build_time.h#newc... base/build_time.h:21: // It will always be in the past. On 2016/02/09 at 00:14:13, M-A Ruel wrote: > It is guaranteed to be in the past in UTC. The time is given with respect to UTC. If you take a look at base/time.h, it indicates that all times are considered relative to the windows epoch: // Time represents an absolute point in coordinated universal time (UTC), // internally represented as microseconds (s/1,000,000) since the Windows epoch // (1601-01-01 00:00:00 UTC). System-dependent clock interface routines are // defined in time_PLATFORM.cc. Regardless of what timezone you're in, Time::Now() will be converted into an internal time - and that will always be /after/ the time that chromium was built.
On 2016/02/08 01:26:37, M-A Ruel wrote: > On 2016/02/06 17:39:01, Dirk Pranke wrote: > > > And not committing this leaves us in a *worse* situation. > > > I don't understand the push back here. > > > > In my view the current code isn't good, but the change doesn't make things all > > that much better (by changing the file from updating once a day to once a > > month). > > You seriously misunderstand this change. Reread: > https://codereview.chromium.org/1641413002/diff/200001/base/build_time.cc I'm sorry, my last comment was essentially gibberish :). Please ignore it. > It changes from "never enforcing EV compliance nor HSTS pinning on chromium > builds on CI" to "enforcing it for 40 days". Don't we only check the build time if the client-side clock is wrong? The clocks should be right on the bots, so I would expect this code path to not fire ... I had other comments but it looks like Zachary is now following a different approach, so let me now look at that patch instead.
why did you go back to generating the header in the build? It seems much simpler as a gclient hook. https://codereview.chromium.org/1641413002/diff/260001/base/build_time.h File base/build_time.h (right): https://codereview.chromium.org/1641413002/diff/260001/base/build_time.h#newc... base/build_time.h:14: // rounded down to midnight at the start of the day in UTC. This should be 5am instead, right? https://codereview.chromium.org/1641413002/diff/260001/base/build_time.h#newc... base/build_time.h:17: // changes. It will, however, be updated whenever //build/util/LASTCHANGED s/LASTCHANGED/LASTCHANGE https://codereview.chromium.org/1641413002/diff/260001/base/build_time_unitte... File base/build_time_unittest.cc (right): https://codereview.chromium.org/1641413002/diff/260001/base/build_time_unitte... base/build_time_unittest.cc:30: // crash. this comment is wrong, now, right? BUILD_DATE will be updated as needed, and you are doing more than just testing that it doesn't crash, with the new test you're adding. https://codereview.chromium.org/1641413002/diff/260001/build/config/build_met... File build/config/build_metadata.gni (right): https://codereview.chromium.org/1641413002/diff/260001/build/config/build_met... build/config/build_metadata.gni:9: dont_embed_build_metadata = false I'm not aware of any GN bots that set this flag ... I think it's safe to delete this file completely.
On 2016/02/09 at 02:46:56, dpranke wrote: > why did you go back to generating the header in the build? > > It seems much simpler as a gclient hook. The hook doesn't have access to out/Default/gen/... etc. Otherwise, have fixed the comments you've pointed out - good catches. Re: bots, after taking a look, I think you're right, so I've completely removed it for this patchset, while creating https://codereview.chromium.org/1681833002 to scrub it out as well. Cheers!
Description was changed from
==========
Makes GetBuildTime behave identically on all build types.
After discussion with maruel and agl, it seems that
(1) for the purposes of build determinism, it's necessary
to be able to arbitrarily set the build time.
(2) for the purposes of continuous integration, longer durations
between cache invalidation are better, but 24h is probably
workable.
(3) for security purposes, timebombs would ideally be as close to
the actual time of the build as possible.
All of these can reasonably be satisfied by setting the build time
to midnight the day of the build. In the case that the build time needs
to be set to a specific value (i.e. to reproduce a build), a define can
be used to set the BUILD_DATE explicitly.
There still exist a number of checks against DONT_EMBED_BUILD_METADATA
at locations where GetBuildTime() is frequently used - I'm not familiar
enough with the contexts where that code is used to say with surety if
those should be removed. In the meantime, I've removed these, however -
if you know of a reason why this is unreasonable, let me know.
While this doesn't actually store current build time in a resource, it
answers the spirit of the bug below - it makes it possible to run that
type of test locally without setting a special flag.
BUG=489490
==========
to
==========
Makes GetBuildTime behave identically on all build types.
After discussion with maruel and agl, it seems that
(1) for the purposes of build determinism, it's necessary
to be able to arbitrarily set the build time.
(2) for the purposes of continuous integration, longer durations
between cache invalidation are better, but 1mo is probably
workable.
(3) for security purposes, timebombs would ideally be as close to
the actual time of the build as possible.
All of these can reasonably be satisfied by setting the build time
to midnight the day of the build. In the case that the build time needs
to be set to a specific value (i.e. to reproduce a build), a define can
be used to set the BUILD_DATE explicitly.
There still exist a number of checks against DONT_EMBED_BUILD_METADATA
at locations where GetBuildTime() is frequently used - I'm not familiar
enough with the contexts where that code is used to say with surety if
those should be removed. In the meantime, I've removed these, however -
if you know of a reason why this is unreasonable, let me know.
While this doesn't actually store current build time in a resource, it
answers the spirit of the bug below - it makes it possible to run that
type of test locally without setting a special flag.
BUG=489490
==========
Description was changed from
==========
Makes GetBuildTime behave identically on all build types.
After discussion with maruel and agl, it seems that
(1) for the purposes of build determinism, it's necessary
to be able to arbitrarily set the build time.
(2) for the purposes of continuous integration, longer durations
between cache invalidation are better, but 1mo is probably
workable.
(3) for security purposes, timebombs would ideally be as close to
the actual time of the build as possible.
All of these can reasonably be satisfied by setting the build time
to midnight the day of the build. In the case that the build time needs
to be set to a specific value (i.e. to reproduce a build), a define can
be used to set the BUILD_DATE explicitly.
There still exist a number of checks against DONT_EMBED_BUILD_METADATA
at locations where GetBuildTime() is frequently used - I'm not familiar
enough with the contexts where that code is used to say with surety if
those should be removed. In the meantime, I've removed these, however -
if you know of a reason why this is unreasonable, let me know.
While this doesn't actually store current build time in a resource, it
answers the spirit of the bug below - it makes it possible to run that
type of test locally without setting a special flag.
BUG=489490
==========
to
==========
Makes GetBuildTime behave identically on all build types.
After discussion with maruel and agl, it seems that
(1) for the purposes of build determinism, it's necessary
to be able to arbitrarily set the build time.
(2) for the purposes of continuous integration, longer durations
between cache invalidation are better, but 1mo is workable.
(3) for security purposes, timebombs would ideally be as close to
the actual time of the build as possible.
All of these can reasonably be satisfied by setting the build time
to midnight the day of the build. In the case that the build time needs
to be set to a specific value (i.e. to reproduce a build), a define can
be used to set the BUILD_DATE explicitly.
There still exist a number of checks against DONT_EMBED_BUILD_METADATA
at locations where GetBuildTime() is frequently used - I'm not familiar
enough with the contexts where that code is used to say with surety if
those should be removed. In the meantime, I've removed these, however -
if you know of a reason why this is unreasonable, let me know.
While this doesn't actually store current build time in a resource, it
answers the spirit of the bug below - it makes it possible to run that
type of test locally without setting a special flag.
BUG=489490
==========
Description was changed from
==========
Makes GetBuildTime behave identically on all build types.
After discussion with maruel and agl, it seems that
(1) for the purposes of build determinism, it's necessary
to be able to arbitrarily set the build time.
(2) for the purposes of continuous integration, longer durations
between cache invalidation are better, but 1mo is workable.
(3) for security purposes, timebombs would ideally be as close to
the actual time of the build as possible.
All of these can reasonably be satisfied by setting the build time
to midnight the day of the build. In the case that the build time needs
to be set to a specific value (i.e. to reproduce a build), a define can
be used to set the BUILD_DATE explicitly.
There still exist a number of checks against DONT_EMBED_BUILD_METADATA
at locations where GetBuildTime() is frequently used - I'm not familiar
enough with the contexts where that code is used to say with surety if
those should be removed. In the meantime, I've removed these, however -
if you know of a reason why this is unreasonable, let me know.
While this doesn't actually store current build time in a resource, it
answers the spirit of the bug below - it makes it possible to run that
type of test locally without setting a special flag.
BUG=489490
==========
to
==========
Makes GetBuildTime behave sanely on all build types.
After discussion with maruel and agl, it seems that
(1) for the purposes of build determinism, it's necessary
to be able to arbitrarily set the build time.
(2) for the purposes of continuous integration, longer duration
between cache invalidation is better, but >=1mo is preferable.
(3) for security purposes, timebombs would ideally be as close to
the actual time of the build as possible. It must be in the past.
(4) HSTS certificate pinning is valid for 70 days. To make CI builds enforce
HTST pinning, <=1mo is preferable.
All of these can reasonably be satisfied by using different settings for CI
versus official builds:
- For official build, the build time is set to 5:00am of the day of the build or
the day before.
- For continuous integration build, the build time is set to the current month.
If the current day is within the first week of the month and last Sunday
wasn't part of the current month, the Sunday of the previous month is used.
This results that cache invalidation happens on a Sunday, which is preferable
from an infrastructure standpoint.
- In the case that the build time needs to be set to a specific value (i.e. to
reproduce a build), the GN/GYP variable 'override_build_date' can be used to
set the BUILD_DATE explicitly. Its format is "Mmm DD YYYY".
The way it is done is:
- Generate $target_gen_dir/generated_build_date.h that defines BUILD_DATE. Its
value depends on if an official build is done or not.
- This step depends on build/util/LASTCHANGE so it is run at every sync.
Most importantly, this change removes the need of both GN/GYP variable
"dont_embed_build_metadata" and C define "DONT_EMBED_BUILD_METADATA"; the build
is always deterministic (up to a month) by default. This removes the risk
oversight of forgetting to set this variable, which already happened.
R=maruel@chromium.org
BUG=489490
==========
Description was changed from
==========
Makes GetBuildTime behave sanely on all build types.
After discussion with maruel and agl, it seems that
(1) for the purposes of build determinism, it's necessary
to be able to arbitrarily set the build time.
(2) for the purposes of continuous integration, longer duration
between cache invalidation is better, but >=1mo is preferable.
(3) for security purposes, timebombs would ideally be as close to
the actual time of the build as possible. It must be in the past.
(4) HSTS certificate pinning is valid for 70 days. To make CI builds enforce
HTST pinning, <=1mo is preferable.
All of these can reasonably be satisfied by using different settings for CI
versus official builds:
- For official build, the build time is set to 5:00am of the day of the build or
the day before.
- For continuous integration build, the build time is set to the current month.
If the current day is within the first week of the month and last Sunday
wasn't part of the current month, the Sunday of the previous month is used.
This results that cache invalidation happens on a Sunday, which is preferable
from an infrastructure standpoint.
- In the case that the build time needs to be set to a specific value (i.e. to
reproduce a build), the GN/GYP variable 'override_build_date' can be used to
set the BUILD_DATE explicitly. Its format is "Mmm DD YYYY".
The way it is done is:
- Generate $target_gen_dir/generated_build_date.h that defines BUILD_DATE. Its
value depends on if an official build is done or not.
- This step depends on build/util/LASTCHANGE so it is run at every sync.
Most importantly, this change removes the need of both GN/GYP variable
"dont_embed_build_metadata" and C define "DONT_EMBED_BUILD_METADATA"; the build
is always deterministic (up to a month) by default. This removes the risk
oversight of forgetting to set this variable, which already happened.
R=maruel@chromium.org
BUG=489490
==========
to
==========
Makes GetBuildTime behave sanely on all build types.
After discussion with maruel and agl, it seems that
(1) for the purposes of build determinism, it's necessary
to be able to arbitrarily set the build time.
(2) for the purposes of continuous integration, longer duration
between cache invalidation is better, but >=1mo is preferable.
(3) for security purposes, timebombs would ideally be as close to
the actual time of the build as possible. It must be in the past.
(4) HSTS certificate pinning is valid for 70 days. To make CI builds enforce
HTST pinning, <=1mo is preferable.
All of these can reasonably be satisfied by using different settings for CI
versus official builds:
- For official build, the build time is set to 5:00am of the day of the build or
the day before.
- For continuous integration build, the build time is set to the current month.
If the current day is within the first week of the month and last Sunday
wasn't part of the current month, the Sunday of the previous month is used.
This results that cache invalidation happens on a Sunday, which is preferable
from an infrastructure standpoint.
- In the case that the build time needs to be set to a specific value (i.e. to
reproduce a build), the GN/GYP variable 'override_build_date' can be used to
set the BUILD_DATE explicitly. Its format is "Mmm DD YYYY".
The way it is done is:
- Generate $target_gen_dir/generated_build_date.h that defines BUILD_DATE. Its
value depends on if an official build is done or not.
- This step depends on build/util/LASTCHANGE so it is run at every sync. The
file is only touched if the content changed to not affect null build.
Most importantly, this change removes the need of both GN/GYP variable
"dont_embed_build_metadata" and C define "DONT_EMBED_BUILD_METADATA"; the build
is always deterministic (up to a month) by default. This removes the risk
oversight of forgetting to set this variable, which already happened.
R=maruel@chromium.org
BUG=489490
==========
lgtm pending win_disable_handle_verifier_hooks. I suspect it's a bad merge
conflict.
Thanks for getting this through!
I've rewritten the CL description to add more context. Feel free to further
edit.
Here's the previous one which had stale information:
---
Makes GetBuildTime behave identically on all build types.
After discussion with maruel and agl, it seems that
(1) for the purposes of build determinism, it's necessary
to be able to arbitrarily set the build time.
(2) for the purposes of continuous integration, longer durations
between cache invalidation are better, but 24h is probably
workable.
(3) for security purposes, timebombs would ideally be as close to
the actual time of the build as possible.
All of these can reasonably be satisfied by setting the build time
to midnight the day of the build. In the case that the build time needs
to be set to a specific value (i.e. to reproduce a build), a define can
be used to set the BUILD_DATE explicitly.
There still exist a number of checks against DONT_EMBED_BUILD_METADATA
at locations where GetBuildTime() is frequently used - I'm not familiar
enough with the contexts where that code is used to say with surety if
those should be removed. In the meantime, I've removed these, however -
if you know of a reason why this is unreasonable, let me know.
While this doesn't actually store current build time in a resource, it
answers the spirit of the bug below - it makes it possible to run that
type of test locally without setting a special flag.
BUG=489490
---
https://codereview.chromium.org/1641413002/diff/320001/base/BUILD.gn
File base/BUILD.gn (right):
https://codereview.chromium.org/1641413002/diff/320001/base/BUILD.gn#newcode33
base/BUILD.gn:33: # Hookless parts of the handle verifier will still function.
What's that?
I continue to think this approach is both overly complicated and not ideal. As I mentioned earlier, I think setting things to the first sunday of the month rather than the current day will have little to no effect; //base gets changed multiple times per day so I would expect little to no reuse of binaries beyond that, although I do think that using the current date could trigger at least one more set of binaries, if the date updates at 5am but base has not changed otherwise. [ Marc-Antoine, if I'm confused about this it would be good to know. ] It's not clear to me that this change will actually have any effect on what the CI bots do in practice, since AFAIK the CI bots have correct system clocks and so we will never check the build time, right? So, I don't think this change actually fixes the referenced bug, although it might make it slightly easier to manually test things? I also continue to think that using the actual build date rather than the date of the last change to //chrome/VERSION is the wrong thing to do, at least by default. If you were to use the checkin date, the build would be completely deterministic, you wouldn't need the manual override option, and the date would more accurately reflect the actual state of the code. I don't think such a change would affect the security properties of the patch in practice, but if it did, IMO it would be a change for the better (unless I'm missing something?) > zforman wrote: > > On 2016/02/09 at 02:46:56, dpranke wrote: > > why did you go back to generating the header in the build? > > > > It seems much simpler as a gclient hook. > > The hook doesn't have access to out/Default/gen/... etc. That makes sense, and I forgot about that. We could put the generated file in //build/util like we do w/ LASTCHANGE, but I suppose the in-GN/GYP approach basically works as well. https://codereview.chromium.org/1641413002/diff/320001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1641413002/diff/320001/base/BUILD.gn#newcode33 base/BUILD.gn:33: # Hookless parts of the handle verifier will still function. On 2016/02/09 13:57:06, M-A Ruel wrote: > What's that? I also am assuming this is a weird/bad merge and needs to go away. https://codereview.chromium.org/1641413002/diff/320001/base/build_time.h File base/build_time.h (right): https://codereview.chromium.org/1641413002/diff/320001/base/build_time.h#newc... base/build_time.h:21: // It will always be in the past. This comment is wrong for non-official builds, since the date may be many days older than the actual build date. https://codereview.chromium.org/1641413002/diff/320001/build/write_build_date... File build/write_build_date_header.py (right): https://codereview.chromium.org/1641413002/diff/320001/build/write_build_date... build/write_build_date_header.py:9: invalidating the build cache shouldn't have major reprecussions. This comment is wrong since it only talks about the non-official case, not the official case or the manually set case.
On 2016/02/09 19:38:03, Dirk Pranke wrote: > I continue to think this approach is both overly complicated and not ideal. > > As I mentioned earlier, I think setting things to the first sunday of the month > rather than the current day will have little to no effect; //base gets changed > multiple times per day so I would expect little to no reuse of binaries beyond > that, although I do think that using the current date could trigger at least one > more set of binaries, if the date updates at 5am but base has not changed > otherwise. > > [ Marc-Antoine, if I'm confused about this it would be good to know. ] Yes you are confused. We don't want to reuse binaries for official builds, period. For non official binaries, we change the date only once per month. >> It's not clear to me that this change will actually have any effect on what the > CI bots do in practice, since AFAIK the CI bots have correct system clocks and > so we will never check the build time, right? No. The CI bots do have proper system clock, and we will now enforce HSTS pinning, leading to potentially more accurate testing. > So, I don't think this change > actually fixes the referenced bug, although it might make it slightly easier to > manually test things? > > I also continue to think that using the actual build date rather than the date > of the last change to //chrome/VERSION is the wrong thing to do, at least by > default. If you were to use the checkin date, the build would be completely > deterministic, you wouldn't need the manual override option, and the date would > more accurately reflect the actual state of the code. I don't think such a > change would affect the security properties of the patch in practice, but if it > did, IMO it would be a change for the better (unless I'm missing something?) Please take over https://code.google.com/p/chromium/issues/detail?id=584880 if you want to continue this approach. As I said, you'll have to discuss with security folks before being able to drive this through as this changes the semantic of BUILD_DATE, unlike this change. https://codereview.chromium.org/1641413002/diff/320001/base/build_time.h File base/build_time.h (right): https://codereview.chromium.org/1641413002/diff/320001/base/build_time.h#newc... base/build_time.h:21: // It will always be in the past. On 2016/02/09 19:38:03, Dirk Pranke wrote: > This comment is wrong for non-official builds, since the date may be many days > older than the actual build date. I agree, the comment is stale since the accuracy is dependent on OFFICIAL_BUILD. https://codereview.chromium.org/1641413002/diff/320001/build/write_build_date... File build/write_build_date_header.py (right): https://codereview.chromium.org/1641413002/diff/320001/build/write_build_date... build/write_build_date_header.py:9: invalidating the build cache shouldn't have major reprecussions. On 2016/02/09 19:38:03, Dirk Pranke wrote: > This comment is wrong since it only talks about the non-official case, not the > official case or the manually set case. True.
On 2016/02/09 19:47:41, M-A Ruel wrote: > On 2016/02/09 19:38:03, Dirk Pranke wrote: > > I continue to think this approach is both overly complicated and not ideal. > > > > As I mentioned earlier, I think setting things to the first sunday of the > month > > rather than the current day will have little to no effect; //base gets changed > > multiple times per day so I would expect little to no reuse of binaries beyond > > that, although I do think that using the current date could trigger at least > one > > more set of binaries, if the date updates at 5am but base has not changed > > otherwise. > > > > [ Marc-Antoine, if I'm confused about this it would be good to know. ] > > Yes you are confused. We don't want to reuse binaries for official builds, > period. > For non official binaries, we change the date only once per month. > How am I confused? First, I wasn't talking about official builds; I recognize that you're not changing that behavior (much as I might like you to do so ... :). Second, if //base changes multiple times a day (which it does), changing the date less frequently will have *no effect* on whether we can reuse the build artifacts; they will have changed because of other commits (apart from the one case I identified). Am I right? > > >> It's not clear to me that this change will actually have any effect on what > the > > CI bots do in practice, since AFAIK the CI bots have correct system clocks and > > so we will never check the build time, right? > > No. The CI bots do have proper system clock, and we will now enforce HSTS > pinning, > leading to potentially more accurate testing. My understanding from lgarron is that we only look at the IsBuildTimely() functions if the check against the system clock fails, and that likely won't happen on the CI bots since the clocks are right, so how does this patch change which code paths will execute? What am I missing here?
On 2016/02/09 20:08:01, Dirk Pranke wrote: > On 2016/02/09 19:47:41, M-A Ruel wrote: > > On 2016/02/09 19:38:03, Dirk Pranke wrote: > > > I continue to think this approach is both overly complicated and not ideal. > > > > > > As I mentioned earlier, I think setting things to the first sunday of the > > month > > > rather than the current day will have little to no effect; //base gets > changed > > > multiple times per day so I would expect little to no reuse of binaries > beyond > > > that, although I do think that using the current date could trigger at least > > one > > > more set of binaries, if the date updates at 5am but base has not changed > > > otherwise. > > > > > > [ Marc-Antoine, if I'm confused about this it would be good to know. ] > > > > Yes you are confused. We don't want to reuse binaries for official builds, > > period. > > For non official binaries, we change the date only once per month. > > > > How am I confused? > > First, I wasn't talking about official builds; I recognize that you're not > changing that behavior (much as I might like you to do so ... :). > > Second, if //base changes multiple times a day (which it does), > changing the date less frequently will have *no effect* on > whether we can reuse the build artifacts; they will have changed > because of other commits (apart from the one case I identified). > > Am I right? The header file is only touched if the content (the date) changed. The date changes once per month. > > >> It's not clear to me that this change will actually have any effect on what > > the > > > CI bots do in practice, since AFAIK the CI bots have correct system clocks > and > > > so we will never check the build time, right? > > > > No. The CI bots do have proper system clock, and we will now enforce HSTS > > pinning, > > leading to potentially more accurate testing. > > My understanding from lgarron is that we only look at the IsBuildTimely() > functions > if the check against the system clock fails, and that likely won't happen on the > CI > bots since the clocks are right, so how does this patch change which code paths > will execute? > > What am I missing here? BUILD_DATE was set to 2008 on the CI. So the code path was never tested.
Thanks very much for the rewrite of the CL description M-A, it looks great :) dpranke: Note that there were code paths that were #ifndef'd out - they probably weren't even being /compiled/ on CI (unless there are bots that have dont_embed_build_metadata = false || official_build = true, which I don't think is the case). https://codereview.chromium.org/1641413002/diff/320001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1641413002/diff/320001/base/BUILD.gn#newcode33 base/BUILD.gn:33: # Hookless parts of the handle verifier will still function. On 2016/02/09 at 19:38:03, Dirk Pranke wrote: > On 2016/02/09 13:57:06, M-A Ruel wrote: > > What's that? > > I also am assuming this is a weird/bad merge and needs to go away. Correct! Will remove. https://codereview.chromium.org/1641413002/diff/320001/base/build_time.h File base/build_time.h (right): https://codereview.chromium.org/1641413002/diff/320001/base/build_time.h#newc... base/build_time.h:21: // It will always be in the past. On 2016/02/09 at 19:47:41, M-A Ruel wrote: > On 2016/02/09 19:38:03, Dirk Pranke wrote: > > This comment is wrong for non-official builds, since the date may be many days > > older than the actual build date. > > I agree, the comment is stale since the accuracy is dependent on OFFICIAL_BUILD. Acknowledged and corrected. https://codereview.chromium.org/1641413002/diff/320001/build/write_build_date... File build/write_build_date_header.py (right): https://codereview.chromium.org/1641413002/diff/320001/build/write_build_date... build/write_build_date_header.py:9: invalidating the build cache shouldn't have major reprecussions. On 2016/02/09 at 19:47:41, M-A Ruel wrote: > On 2016/02/09 19:38:03, Dirk Pranke wrote: > > This comment is wrong since it only talks about the non-official case, not the > > official case or the manually set case. > > True. Acknowledged and updated.
lgtm https://codereview.chromium.org/1641413002/diff/340001/build/write_build_date... File build/write_build_date_header.py (right): https://codereview.chromium.org/1641413002/diff/340001/build/write_build_date... build/write_build_date_header.py:52: return "{:%b %d %Y}".format(datetime.date(year, month, day)) single quote https://codereview.chromium.org/1641413002/diff/340001/build/write_build_date... build/write_build_date_header.py:74: current_contents = "" single quote
Fixed use of " + hit a few more that snuck in. https://codereview.chromium.org/1641413002/diff/340001/build/write_build_date... File build/write_build_date_header.py (right): https://codereview.chromium.org/1641413002/diff/340001/build/write_build_date... build/write_build_date_header.py:52: return "{:%b %d %Y}".format(datetime.date(year, month, day)) On 2016/02/09 at 21:00:58, M-A Ruel wrote: > single quote Fixed https://codereview.chromium.org/1641413002/diff/340001/build/write_build_date... build/write_build_date_header.py:74: current_contents = "" On 2016/02/09 at 21:00:58, M-A Ruel wrote: > single quote Fixed
lgtm
agl, thakis: For owners approval, I need LGTMs from you. Could you please review, and LGTM if you are happy with this CL? Thanks, Zac
lgtm
This is a nice compromise, lgtm except for the build/config/posix/BUILD.gn change which I don't understand and which looks somewhat unrelated to this CL. https://codereview.chromium.org/1641413002/diff/360001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1641413002/diff/360001/base/BUILD.gn#newcode30 base/BUILD.gn:30: override_build_date = "N/A" is this still needed? doesn't look like it? https://codereview.chromium.org/1641413002/diff/360001/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/1641413002/diff/360001/base/base.gyp#newcode998 base/base.gyp:998: 'target_name': 'base_build_date', This also needs a 'hard_dependency': 1, line because it generates header files. (Not needed in gn.) https://codereview.chromium.org/1641413002/diff/360001/build/config/posix/BUI... File build/config/posix/BUILD.gn (left): https://codereview.chromium.org/1641413002/diff/360001/build/config/posix/BUI... build/config/posix/BUILD.gn:19: # See crbug.com/580103 for some discussion of why this was necessary. ...won't this change regress things? https://codereview.chromium.org/1641413002/diff/360001/build/write_build_date... File build/write_build_date_header.py (right): https://codereview.chromium.org/1641413002/diff/360001/build/write_build_date... build/write_build_date_header.py:26: def GetFirstSundayOfMonth(month, year): nit: year, month order seems more natural https://codereview.chromium.org/1641413002/diff/360001/build/write_build_date... build/write_build_date_header.py:35: gmt_now = datetime.datetime.utcnow() i'd make gmt_now a parameter, then you could have an assert-like test like mentioned for this too (this function has quite a bit of logic...) https://codereview.chromium.org/1641413002/diff/360001/build/write_build_date... build/write_build_date_header.py:53: nit: For small scripts like this were having a dedicated _unittest.py file doesn't seem worth it, I like to put in a few tests inline like assert GetFirstSundayOfMonth(13, 2014) == 4 # or whatever the expected value is That way, the tests run every time the script runs and there's at least some testing (and they won't noticeable slow down the script)
M-A and I discussed aspects of this off-CL, and agreed that if we were to have the non-official bots also use the current date (rather than the first sunday of the month), then that would have the same functional effect as landing a CL to //base/build_time.cc every morning at 5am, and that that might have enough of an unpleasant effect on the swarming cache to make anyone working at that time kinda grumpy. I'm still not convinced that all of this complexity is worth it, and I still think that switching to a //chrome/VERSION-based approach would be better, but I won't block this CL as-is (as long as it incorporates the suggestions from the other reviewers, of course).
Thanks for the comments - I especially like the method of inline testing of python. https://codereview.chromium.org/1641413002/diff/360001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1641413002/diff/360001/base/BUILD.gn#newcode30 base/BUILD.gn:30: override_build_date = "N/A" On 2016/02/09 at 22:53:49, Nico wrote: > is this still needed? doesn't look like it? I think it's still nice to allow users to override the build date (to be able to reproduce a build exactly months later). https://codereview.chromium.org/1641413002/diff/360001/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/1641413002/diff/360001/base/base.gyp#newcode998 base/base.gyp:998: 'target_name': 'base_build_date', On 2016/02/09 at 22:53:49, Nico wrote: > This also needs a > > 'hard_dependency': 1, > > line because it generates header files. (Not needed in gn.) Thanks for the save :) https://codereview.chromium.org/1641413002/diff/360001/build/config/posix/BUI... File build/config/posix/BUILD.gn (left): https://codereview.chromium.org/1641413002/diff/360001/build/config/posix/BUI... build/config/posix/BUILD.gn:19: # See crbug.com/580103 for some discussion of why this was necessary. On 2016/02/09 at 22:53:49, Nico wrote: > ...won't this change regress things? No, because of a CL dsansome submitted a few days ago: https://codereview.chromium.org/1658553002 (and its followups). This CL can nuke dont_embed_build_metadata completely. Edit: Wait, yes it will. I removed the wrong path. Corrected! https://codereview.chromium.org/1641413002/diff/360001/build/write_build_date... File build/write_build_date_header.py (right): https://codereview.chromium.org/1641413002/diff/360001/build/write_build_date... build/write_build_date_header.py:26: def GetFirstSundayOfMonth(month, year): On 2016/02/09 at 22:53:49, Nico wrote: > nit: year, month order seems more natural Fair - I think that's probably true in general. I was thinking d/m/y, but y/m/d is what is used in general. https://codereview.chromium.org/1641413002/diff/360001/build/write_build_date... build/write_build_date_header.py:35: gmt_now = datetime.datetime.utcnow() On 2016/02/09 at 22:53:49, Nico wrote: > i'd make gmt_now a parameter, then you could have an assert-like test like mentioned for this too (this function has quite a bit of logic...) I like this idea, will implement. https://codereview.chromium.org/1641413002/diff/360001/build/write_build_date... build/write_build_date_header.py:53: On 2016/02/09 at 22:53:49, Nico wrote: > nit: For small scripts like this were having a dedicated _unittest.py file doesn't seem worth it, I like to put in a few tests inline like > > assert GetFirstSundayOfMonth(13, 2014) == 4 # or whatever the expected value is > > That way, the tests run every time the script runs and there's at least some testing (and they won't noticeable slow down the script) I like this idea. Will implement.
lgtm I'd consider landing this in three pieces: * the sysroot rebase bit * the date bit * removing the dont_embed_metadata flag but that's only to reduce risk. If you want to land the whole thing in one piece, that's fine too. https://codereview.chromium.org/1641413002/diff/380001/build/config/posix/BUI... File build/config/posix/BUILD.gn (right): https://codereview.chromium.org/1641413002/diff/380001/build/config/posix/BUI... build/config/posix/BUILD.gn:16: cflags = [ "--sysroot=" + rebase_path(sysroot, root_build_dir) ] fwiw i'd land this change first, in a separate cl, in case codesearch (or something else) can't handle it yet after all – then only that change will be reverted, not the whole thing
(also, maybe you can update the timestamp bits on https://www.chromium.org/developers/testing/isolated-testing/deterministic-bu... to explain how things work now, after this lands)
On 2016/02/10 at 17:30:11, thakis wrote: > (also, maybe you can update the timestamp bits on https://www.chromium.org/developers/testing/isolated-testing/deterministic-bu... to explain how things work now, after this lands) Will do. > I'd consider landing this in three pieces: Because time constraints I'll land the whole thing - otherwise I totally agree. CodeSearch should already be using relative paths (dsansome removed the dont_embed_build_metadata=false in the bots), so this shouldn't regress that. Cheers!
The CQ bit was checked by zforman@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from lgarron@chromium.org, felt@chromium.org, maruel@chromium.org, agl@chromium.org Link to the patchset: https://codereview.chromium.org/1641413002/#ps380001 (title: "Response to #96")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641413002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641413002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
On 2016/02/11 02:10:07, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) --- CUT HERE --- ninja -C E:\b\build\slave\win\build\src\out\Debug accessibility_unittests accessibility_unittests_run app_shell_unittests app_shell_unittests_run aura_unittests aura_unittests_run base_unittests base_unittests_run battor_agent_unittests battor_agent_unittests_run browser_tests browser_tests_run cacheinvalidation_unittests cacheinvalidation_unittests_run cast_unittests cast_unittests_run cc_unittests cc_unittests_run chrome_app_unittests chrome_app_unittests_run chrome_elf_unittests chromedriver_unittests chromedriver_unittests_run chromium_builder_tests components_unittests components_unittests_run compositor_unittests compositor_unittests_run content_browsertests content_browsertests_run content_unittests content_unittests_run courgette_unittests courgette_unittests_run crash_service crypto_unittests crypto_unittests_run events_unittests events_unittests_run extensions_browsertests extensions_browsertests_run extensions_unittests extensions_unittests_run gcm_unit_tests gcm_unit_tests_run gn_unittests gn_unittests_run gpu_unittests gpu_unittests_run installer_util_unittests installer_util_unittests_run interactive_ui_tests interactive_ui_tests_run ipc_tests ipc_tests_run jingle_unittests jingle_unittests_run media_blink_unittests media_blink_unittests_run media_unittests media_unittests_run midi_unittests midi_unittests_run mojo_common_unittests mojo_common_unittests_run mojo_public_bindings_unittests mojo_public_bindings_unittests_run mojo_public_environment_unittests mojo_public_environment_unittests_run mojo_public_system_unittests mojo_public_system_unittests_run mojo_public_utility_unittests mojo_public_utility_unittests_run mojo_system_unittests nacl_loader_unittests nacl_loader_unittests_run net_unittests net_unittests_run ppapi_unittests printing_unittests printing_unittests_run remoting_unittests remoting_unittests_run sbox_integration_tests sbox_integration_tests_run sbox_unittests sbox_unittests_run sbox_validation_tests sbox_validation_tests_run setup_unittests setup_unittests_run skia_unittests skia_unittests_run sql_unittests sql_unittests_run sync_integration_tests sync_integration_tests_run sync_unit_tests sync_unit_tests_run telemetry_gpu_unittests_run telemetry_perf_unittests_run telemetry_unittests_run ui_base_unittests ui_base_unittests_run ui_touch_selection_unittests ui_touch_selection_unittests_run unit_tests unit_tests_run url_unittests url_unittests_run views_unittests wm_unittests wm_unittests_run -j80 -d explain ninja: Entering directory `E:\b\build\slave\win\build\src\out\Debug' [1/5] ACTION Create installer archive [2/5] STAMP obj\chrome\installer\mini_installer.actions_rules_copies.stamp [3/5] RC obj\chrome\installer\mini_installer.gen\packed_files.rc [4/5] LINK_EMBED mini_installer.exe [5/5] STAMP obj\build\chromium_builder_tests.actions_depends.stamp ninja explain: restat of output chrome.7z older than most recent input ../../out/Debug/ash_with_content.dll (0 vs 476855498) ninja explain: obj/chrome/installer/mini_installer.gen/packed_files.rc is dirty ninja explain: chrome.7z is dirty ninja explain: setup.ex_ is dirty ninja explain: obj/chrome/installer/mini_installer.gen/packed_files.rc is dirty ninja explain: obj/chrome/installer/obj/chrome/installer/mini_installer.gen/mini_installer.packed_files.res is dirty ninja explain: mini_installer.exe is dirty ninja explain: obj/build/chromium_builder_tests.actions_depends.stamp is dirty Failing build because ninja reported work to do. This means that after completing a compile, another was run and it resulted in still having work to do (that is, a no-op build wasn't a no-op). Consult the first "ninja explain:" line for a likely culprit. --- CUT HERE --- I'm not sure what could cause that, chrome.7z has a timestamp of 0. Does it mean it was missing?
On 2016/02/11 at 02:33:24, maruel wrote: > On 2016/02/11 02:10:07, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) > > --- CUT HERE --- > ninja -C E:\b\build\slave\win\build\src\out\Debug accessibility_unittests accessibility_unittests_run app_shell_unittests app_shell_unittests_run aura_unittests aura_unittests_run base_unittests base_unittests_run battor_agent_unittests battor_agent_unittests_run browser_tests browser_tests_run cacheinvalidation_unittests cacheinvalidation_unittests_run cast_unittests cast_unittests_run cc_unittests cc_unittests_run chrome_app_unittests chrome_app_unittests_run chrome_elf_unittests chromedriver_unittests chromedriver_unittests_run chromium_builder_tests components_unittests components_unittests_run compositor_unittests compositor_unittests_run content_browsertests content_browsertests_run content_unittests content_unittests_run courgette_unittests courgette_unittests_run crash_service crypto_unittests crypto_unittests_run events_unittests events_unittests_run extensions_browsertests extensions_browsertests_run extensions_unittests extensions_unittests_run gcm_unit_tests gcm_unit_tests_run gn_unittests gn_unittests_run gpu_unittests gpu_unittests_run installer_util_unittests installer_util_unittests_run interactive_ui_tests interactive_ui_tests_run ipc_tests ipc_tests_run jingle_unittests jingle_unittests_run media_blink_unittests media_blink_unittests_run media_unittests media_unittests_run midi_unittests midi_unittests_run mojo_common_unittests mojo_common_unittests_run mojo_public_bindings_unittests mojo_public_bindings_unittests_run mojo_public_environment_unittests mojo_public_environment_unittests_run mojo_public_system_unittests mojo_public_system_unittests_run mojo_public_utility_unittests mojo_public_utility_unittests_run mojo_system_unittests nacl_loader_unittests nacl_loader_unittests_run net_unittests net_unittests_run ppapi_unittests printing_unittests printing_unittests_run remoting_unittests remoting_unittests_run sbox_integration_tests sbox_integration_tests_run sbox_unittests sbox_unittests_run sbox_validation_tests sbox_validation_tests_run setup_unittests setup_unittests_run skia_unittests skia_unittests_run sql_unittests sql_unittests_run sync_integration_tests sync_integration_tests_run sync_unit_tests sync_unit_tests_run telemetry_gpu_unittests_run telemetry_perf_unittests_run telemetry_unittests_run ui_base_unittests ui_base_unittests_run ui_touch_selection_unittests ui_touch_selection_unittests_run unit_tests unit_tests_run url_unittests url_unittests_run views_unittests wm_unittests wm_unittests_run -j80 -d explain > ninja: Entering directory `E:\b\build\slave\win\build\src\out\Debug' > [1/5] ACTION Create installer archive > [2/5] STAMP obj\chrome\installer\mini_installer.actions_rules_copies.stamp > [3/5] RC obj\chrome\installer\mini_installer.gen\packed_files.rc > [4/5] LINK_EMBED mini_installer.exe > [5/5] STAMP obj\build\chromium_builder_tests.actions_depends.stamp > ninja explain: restat of output chrome.7z older than most recent input ../../out/Debug/ash_with_content.dll (0 vs 476855498) > ninja explain: obj/chrome/installer/mini_installer.gen/packed_files.rc is dirty > ninja explain: chrome.7z is dirty > ninja explain: setup.ex_ is dirty > ninja explain: obj/chrome/installer/mini_installer.gen/packed_files.rc is dirty > ninja explain: obj/chrome/installer/obj/chrome/installer/mini_installer.gen/mini_installer.packed_files.res is dirty > ninja explain: mini_installer.exe is dirty > ninja explain: obj/build/chromium_builder_tests.actions_depends.stamp is dirty > Failing build because ninja reported work to do. > This means that after completing a compile, another was run and > it resulted in still having work to do (that is, a no-op build > wasn't a no-op). Consult the first "ninja explain:" line for a > likely culprit. > --- CUT HERE --- > > I'm not sure what could cause that, chrome.7z has a timestamp of 0. Does it mean it was missing? Yeah, I'm pretty confused too. Nothing I did should've affected it. I'm doing another try to see...
On 2016/02/11 at 02:40:21, Zachary Forman wrote: > On 2016/02/11 at 02:33:24, maruel wrote: > > On 2016/02/11 02:10:07, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) > > > > --- CUT HERE --- > > ninja -C E:\b\build\slave\win\build\src\out\Debug accessibility_unittests accessibility_unittests_run app_shell_unittests app_shell_unittests_run aura_unittests aura_unittests_run base_unittests base_unittests_run battor_agent_unittests battor_agent_unittests_run browser_tests browser_tests_run cacheinvalidation_unittests cacheinvalidation_unittests_run cast_unittests cast_unittests_run cc_unittests cc_unittests_run chrome_app_unittests chrome_app_unittests_run chrome_elf_unittests chromedriver_unittests chromedriver_unittests_run chromium_builder_tests components_unittests components_unittests_run compositor_unittests compositor_unittests_run content_browsertests content_browsertests_run content_unittests content_unittests_run courgette_unittests courgette_unittests_run crash_service crypto_unittests crypto_unittests_run events_unittests events_unittests_run extensions_browsertests extensions_browsertests_run extensions_unittests extensions_unittests_run gcm_unit_tests gcm_unit_tests_run gn_unittests gn_unittests_run gpu_unittests gpu_unittests_run installer_util_unittests installer_util_unittests_run interactive_ui_tests interactive_ui_tests_run ipc_tests ipc_tests_run jingle_unittests jingle_unittests_run media_blink_unittests media_blink_unittests_run media_unittests media_unittests_run midi_unittests midi_unittests_run mojo_common_unittests mojo_common_unittests_run mojo_public_bindings_unittests mojo_public_bindings_unittests_run mojo_public_environment_unittests mojo_public_environment_unittests_run mojo_public_system_unittests mojo_public_system_unittests_run mojo_public_utility_unittests mojo_public_utility_unittests_run mojo_system_unittests nacl_loader_unittests nacl_loader_unittests_run net_unittests net_unittests_run ppapi_unittests printing_unittests printing_unittests_run remoting_unittests remoting_unittests_run sbox_integration_tests sbox_integration_tests_run sbox_unittests sbox_unittests_run sbox_validation_tests sbox_validation_tests_run setup_unittests setup_unittests_run skia_unittests skia_unittests_run sql_unittests sql_unittests_run sync_integration_tests sync_integration_tests_run sync_unit_tests sync_unit_tests_run telemetry_gpu_unittests_run telemetry_perf_unittests_run telemetry_unittests_run ui_base_unittests ui_base_unittests_run ui_touch_selection_unittests ui_touch_selection_unittests_run unit_tests unit_tests_run url_unittests url_unittests_run views_unittests wm_unittests wm_unittests_run -j80 -d explain > > ninja: Entering directory `E:\b\build\slave\win\build\src\out\Debug' > > [1/5] ACTION Create installer archive > > [2/5] STAMP obj\chrome\installer\mini_installer.actions_rules_copies.stamp > > [3/5] RC obj\chrome\installer\mini_installer.gen\packed_files.rc > > [4/5] LINK_EMBED mini_installer.exe > > [5/5] STAMP obj\build\chromium_builder_tests.actions_depends.stamp > > ninja explain: restat of output chrome.7z older than most recent input ../../out/Debug/ash_with_content.dll (0 vs 476855498) > > ninja explain: obj/chrome/installer/mini_installer.gen/packed_files.rc is dirty > > ninja explain: chrome.7z is dirty > > ninja explain: setup.ex_ is dirty > > ninja explain: obj/chrome/installer/mini_installer.gen/packed_files.rc is dirty > > ninja explain: obj/chrome/installer/obj/chrome/installer/mini_installer.gen/mini_installer.packed_files.res is dirty > > ninja explain: mini_installer.exe is dirty > > ninja explain: obj/build/chromium_builder_tests.actions_depends.stamp is dirty > > Failing build because ninja reported work to do. > > This means that after completing a compile, another was run and > > it resulted in still having work to do (that is, a no-op build > > wasn't a no-op). Consult the first "ninja explain:" line for a > > likely culprit. > > --- CUT HERE --- > > > > I'm not sure what could cause that, chrome.7z has a timestamp of 0. Does it mean it was missing? > > Yeah, I'm pretty confused too. Nothing I did should've affected it. I'm doing another try to see... Most recent try failed for the same reason. Guess there's something here that somehow breaks (one of) those 5 things? :/
I tried to reproduce but got a surprising failure:
E:\src\chromium\src>ninja -C out/Release mini_installer
ninja: Entering directory `out/Release'
[6149/21255] RULE Assembling nacl_switch_unwind_win.asm
to..._64\service_runtime_x86_64.gen\nacl_switch_unwind_win.obj.
Assembling: nacl_switch_unwind_win.asm
[21241/21255] ACTION Create installer archive
FAILED: E:\src\depot_tools\python276_bin\python.exe gyp-win-tool action-wrapper
environment.x86
mini_installer_target_installer_archive_77d231bc8c72d7f6287cd82d1076061f..rsp
..\..\chrome\installer
Traceback (most recent call last):
File "../tools/build/win/create_installer_archive.py", line 671, in <module>
sys.exit(main(options))
File "../tools/build/win/create_installer_archive.py", line 587, in main
current_build_number, prev_build_number)
File "../tools/build/win/create_installer_archive.py", line 294, in
CreateArchiveFile
options.verbose)
File "../tools/build/win/create_installer_archive.py", line 92, in
CompressUsingLZMA
RunSystemCommand(cmd, verbose)
File "../tools/build/win/create_installer_archive.py", line 204, in
RunSystemCommand
(e.cmd, e.returncode, e.output))
Exception: Error while running cmd:
['..\\..\\out\\Release\\..\\..\\third_party\\lzma_sdk\\Executable\\7za.exe',
'a', '-t7z', '-m0=BCJ2', '-m1=LZMA:d27:fb128', '-m2=LZMA:d22:fb128:mf=bt2',
'-m3=LZMA:d22:fb128:mf=bt2', '-mb0:1', '-mb0s1:2', '-mb0s2:3',
'..\\..\\out\\Release\\chrome.packed.7z', '..\\..\\out\\Release\\chrome.7z']
Exit code: 8
Command output:
7-Zip (A) 4.42 Copyright (c) 1999-2006 Igor Pavlov 2006-05-14
Scanning
Creating archive ..\..\out\Release\chrome.packed.7z
Compressing chrome.7z
Error:
ERROR: Can't allocate required memory!
----
This means we are getting close to not being able to run 7zip anymore. I
confirmed the 7za.exe executable is 32 bits. I filed
https://code.google.com/p/chromium/issues/detail?id=586201
On 2016/02/11 at 18:08:09, maruel wrote: > I tried to reproduce but got a surprising failure: > > E:\src\chromium\src>ninja -C out/Release mini_installer > ninja: Entering directory `out/Release' > [6149/21255] RULE Assembling nacl_switch_unwind_win.asm to..._64\service_runtime_x86_64.gen\nacl_switch_unwind_win.obj. > Assembling: nacl_switch_unwind_win.asm > [21241/21255] ACTION Create installer archive > FAILED: E:\src\depot_tools\python276_bin\python.exe gyp-win-tool action-wrapper environment.x86 mini_installer_target_installer_archive_77d231bc8c72d7f6287cd82d1076061f..rsp ..\..\chrome\installer > Traceback (most recent call last): > File "../tools/build/win/create_installer_archive.py", line 671, in <module> > sys.exit(main(options)) > File "../tools/build/win/create_installer_archive.py", line 587, in main > current_build_number, prev_build_number) > File "../tools/build/win/create_installer_archive.py", line 294, in CreateArchiveFile > options.verbose) > File "../tools/build/win/create_installer_archive.py", line 92, in CompressUsingLZMA > RunSystemCommand(cmd, verbose) > File "../tools/build/win/create_installer_archive.py", line 204, in RunSystemCommand > (e.cmd, e.returncode, e.output)) > Exception: Error while running cmd: ['..\\..\\out\\Release\\..\\..\\third_party\\lzma_sdk\\Executable\\7za.exe', 'a', '-t7z', '-m0=BCJ2', '-m1=LZMA:d27:fb128', '-m2=LZMA:d22:fb128:mf=bt2', '-m3=LZMA:d22:fb128:mf=bt2', '-mb0:1', '-mb0s1:2', '-mb0s2:3', '..\\..\\out\\Release\\chrome.packed.7z', '..\\..\\out\\Release\\chrome.7z'] > Exit code: 8 > Command output: > > 7-Zip (A) 4.42 Copyright (c) 1999-2006 Igor Pavlov 2006-05-14 > Scanning > > Creating archive ..\..\out\Release\chrome.packed.7z > > Compressing chrome.7z > Error: > > > ERROR: Can't allocate required memory! > > ---- > > This means we are getting close to not being able to run 7zip anymore. I confirmed the 7za.exe executable is 32 bits. I filed https://code.google.com/p/chromium/issues/detail?id=586201 That's a pretty impressive error.
The CQ bit was checked by mithro@mithis.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641413002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641413002/380001
Message was sent while issue was closed.
Description was changed from
==========
Makes GetBuildTime behave sanely on all build types.
After discussion with maruel and agl, it seems that
(1) for the purposes of build determinism, it's necessary
to be able to arbitrarily set the build time.
(2) for the purposes of continuous integration, longer duration
between cache invalidation is better, but >=1mo is preferable.
(3) for security purposes, timebombs would ideally be as close to
the actual time of the build as possible. It must be in the past.
(4) HSTS certificate pinning is valid for 70 days. To make CI builds enforce
HTST pinning, <=1mo is preferable.
All of these can reasonably be satisfied by using different settings for CI
versus official builds:
- For official build, the build time is set to 5:00am of the day of the build or
the day before.
- For continuous integration build, the build time is set to the current month.
If the current day is within the first week of the month and last Sunday
wasn't part of the current month, the Sunday of the previous month is used.
This results that cache invalidation happens on a Sunday, which is preferable
from an infrastructure standpoint.
- In the case that the build time needs to be set to a specific value (i.e. to
reproduce a build), the GN/GYP variable 'override_build_date' can be used to
set the BUILD_DATE explicitly. Its format is "Mmm DD YYYY".
The way it is done is:
- Generate $target_gen_dir/generated_build_date.h that defines BUILD_DATE. Its
value depends on if an official build is done or not.
- This step depends on build/util/LASTCHANGE so it is run at every sync. The
file is only touched if the content changed to not affect null build.
Most importantly, this change removes the need of both GN/GYP variable
"dont_embed_build_metadata" and C define "DONT_EMBED_BUILD_METADATA"; the build
is always deterministic (up to a month) by default. This removes the risk
oversight of forgetting to set this variable, which already happened.
R=maruel@chromium.org
BUG=489490
==========
to
==========
Makes GetBuildTime behave sanely on all build types.
After discussion with maruel and agl, it seems that
(1) for the purposes of build determinism, it's necessary
to be able to arbitrarily set the build time.
(2) for the purposes of continuous integration, longer duration
between cache invalidation is better, but >=1mo is preferable.
(3) for security purposes, timebombs would ideally be as close to
the actual time of the build as possible. It must be in the past.
(4) HSTS certificate pinning is valid for 70 days. To make CI builds enforce
HTST pinning, <=1mo is preferable.
All of these can reasonably be satisfied by using different settings for CI
versus official builds:
- For official build, the build time is set to 5:00am of the day of the build or
the day before.
- For continuous integration build, the build time is set to the current month.
If the current day is within the first week of the month and last Sunday
wasn't part of the current month, the Sunday of the previous month is used.
This results that cache invalidation happens on a Sunday, which is preferable
from an infrastructure standpoint.
- In the case that the build time needs to be set to a specific value (i.e. to
reproduce a build), the GN/GYP variable 'override_build_date' can be used to
set the BUILD_DATE explicitly. Its format is "Mmm DD YYYY".
The way it is done is:
- Generate $target_gen_dir/generated_build_date.h that defines BUILD_DATE. Its
value depends on if an official build is done or not.
- This step depends on build/util/LASTCHANGE so it is run at every sync. The
file is only touched if the content changed to not affect null build.
Most importantly, this change removes the need of both GN/GYP variable
"dont_embed_build_metadata" and C define "DONT_EMBED_BUILD_METADATA"; the build
is always deterministic (up to a month) by default. This removes the risk
oversight of forgetting to set this variable, which already happened.
R=maruel@chromium.org
BUG=489490
==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
this probably caused https://code.google.com/p/chromium/issues/detail?id=586702&thanks=586702&ts=1...
Message was sent while issue was closed.
Description was changed from
==========
Makes GetBuildTime behave sanely on all build types.
After discussion with maruel and agl, it seems that
(1) for the purposes of build determinism, it's necessary
to be able to arbitrarily set the build time.
(2) for the purposes of continuous integration, longer duration
between cache invalidation is better, but >=1mo is preferable.
(3) for security purposes, timebombs would ideally be as close to
the actual time of the build as possible. It must be in the past.
(4) HSTS certificate pinning is valid for 70 days. To make CI builds enforce
HTST pinning, <=1mo is preferable.
All of these can reasonably be satisfied by using different settings for CI
versus official builds:
- For official build, the build time is set to 5:00am of the day of the build or
the day before.
- For continuous integration build, the build time is set to the current month.
If the current day is within the first week of the month and last Sunday
wasn't part of the current month, the Sunday of the previous month is used.
This results that cache invalidation happens on a Sunday, which is preferable
from an infrastructure standpoint.
- In the case that the build time needs to be set to a specific value (i.e. to
reproduce a build), the GN/GYP variable 'override_build_date' can be used to
set the BUILD_DATE explicitly. Its format is "Mmm DD YYYY".
The way it is done is:
- Generate $target_gen_dir/generated_build_date.h that defines BUILD_DATE. Its
value depends on if an official build is done or not.
- This step depends on build/util/LASTCHANGE so it is run at every sync. The
file is only touched if the content changed to not affect null build.
Most importantly, this change removes the need of both GN/GYP variable
"dont_embed_build_metadata" and C define "DONT_EMBED_BUILD_METADATA"; the build
is always deterministic (up to a month) by default. This removes the risk
oversight of forgetting to set this variable, which already happened.
R=maruel@chromium.org
BUG=489490
==========
to
==========
Makes GetBuildTime behave sanely on all build types.
After discussion with maruel and agl, it seems that
(1) for the purposes of build determinism, it's necessary
to be able to arbitrarily set the build time.
(2) for the purposes of continuous integration, longer duration
between cache invalidation is better, but >=1mo is preferable.
(3) for security purposes, timebombs would ideally be as close to
the actual time of the build as possible. It must be in the past.
(4) HSTS certificate pinning is valid for 70 days. To make CI builds enforce
HTST pinning, <=1mo is preferable.
All of these can reasonably be satisfied by using different settings for CI
versus official builds:
- For official build, the build time is set to 5:00am of the day of the build or
the day before.
- For continuous integration build, the build time is set to the current month.
If the current day is within the first week of the month and last Sunday
wasn't part of the current month, the Sunday of the previous month is used.
This results that cache invalidation happens on a Sunday, which is preferable
from an infrastructure standpoint.
- In the case that the build time needs to be set to a specific value (i.e. to
reproduce a build), the GN/GYP variable 'override_build_date' can be used to
set the BUILD_DATE explicitly. Its format is "Mmm DD YYYY".
The way it is done is:
- Generate $target_gen_dir/generated_build_date.h that defines BUILD_DATE. Its
value depends on if an official build is done or not.
- This step depends on build/util/LASTCHANGE so it is run at every sync. The
file is only touched if the content changed to not affect null build.
Most importantly, this change removes the need of both GN/GYP variable
"dont_embed_build_metadata" and C define "DONT_EMBED_BUILD_METADATA"; the build
is always deterministic (up to a month) by default. This removes the risk
oversight of forgetting to set this variable, which already happened.
R=maruel@chromium.org
BUG=489490
Committed: https://crrev.com/08d91b75212b6592f05ff993d5a71c0f5a546563
Cr-Commit-Position: refs/heads/master@{#375136}
==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/08d91b75212b6592f05ff993d5a71c0f5a546563 Cr-Commit-Position: refs/heads/master@{#375136} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
