Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1846713002:
Improve the error message in BuildTime.InThePast to help figure out the problem. (Closed)
DescriptionFix latent bug in GetBuildTime().
- Change the generated_build_date.h to contain the full timestamp, it makes the
code less brittle.
- For official builds before 5:00am, it would generate a build time in the
future. Fix this.
- Improve the error message in BuildTime.InThePast.
- Add test case to ensure the build is not more than 45 days old.
- Remove dummy test.
- Improve comments in base/BUILD.gn and base/base.gyp.
- Add functional write_build_date_header.py --help.
This change builds upon work in https://codereview.chromium.org/1641413002.
R=thakis@chromium.org,felt@chromium.org,lgarron@chromium.org
BUG=587694
Committed: https://crrev.com/1c9b02233631b0ba9b096d79bf5b8d71a370dbb0
Cr-Commit-Position: refs/heads/master@{#384991}
Patch Set 1 #Patch Set 2 : With fix #
Total comments: 3
Patch Set 3 : Rewrite #
Total comments: 5
Patch Set 4 : Improved comment #
Messages
Total messages: 31 (11 generated)
Ah, the error is silly, let me fix it while at it.
Thanks! https://codereview.chromium.org/1846713002/diff/20001/base/build_time_unittes... File base/build_time_unittest.cc (left): https://codereview.chromium.org/1846713002/diff/20001/base/build_time_unittes... base/build_time_unittest.cc:19: TEST(BuildTime, TimeLooksValid) { what's wrong with this test? …oh. Yes, this is not a useful test. https://codereview.chromium.org/1846713002/diff/20001/build/write_build_date_... File build/write_build_date_header.py (right): https://codereview.chromium.org/1846713002/diff/20001/build/write_build_date_... build/write_build_date_header.py:82: # See //base/build_time.cc. This should also refer to the script that has the constant 5 … and the other script should have a comment pointing to this one.
https://codereview.chromium.org/1846713002/diff/20001/build/write_build_date_... File build/write_build_date_header.py (right): https://codereview.chromium.org/1846713002/diff/20001/build/write_build_date_... build/write_build_date_header.py:82: # See //base/build_time.cc. On 2016/03/30 20:34:00, Nico wrote: > This should also refer to the script that has the constant 5 … and the other > script should have a comment pointing to this one. Humm what about changing the script to output the hour in the constant? I guess I'll just do that.
Description was changed from ========== Improve the error message in BuildTime.InThePast to help figure out the problem. Remove dummy test. R=thakis@chromium.org BUG=587694 ========== to ========== Fix latent bug in GetBuildTime(). - For official builds before 5:00am, it would generate a build time in the future. - Improve the error message in BuildTime.InThePast. - Add unit test to ensure the build is not too old. - Remove dummy test. R=thakis@chromium.org BUG=587694 ==========
Description was changed from ========== Fix latent bug in GetBuildTime(). - For official builds before 5:00am, it would generate a build time in the future. - Improve the error message in BuildTime.InThePast. - Add unit test to ensure the build is not too old. - Remove dummy test. R=thakis@chromium.org BUG=587694 ========== to ========== Fix latent bug in GetBuildTime(). - Change the generated_build_date.h to contain the full timestamp, it makes the code less brittle. - For official builds before 5:00am, it would generate a build time in the future. Fix this. - Improve the error message in BuildTime.InThePast. - Add test case to ensure the build is not more than 45 days old. - Remove dummy test. R=thakis@chromium.org,felt@chromium.org,lgarron@chromium.org BUG=587694 ==========
Description was changed from ========== Fix latent bug in GetBuildTime(). - Change the generated_build_date.h to contain the full timestamp, it makes the code less brittle. - For official builds before 5:00am, it would generate a build time in the future. Fix this. - Improve the error message in BuildTime.InThePast. - Add test case to ensure the build is not more than 45 days old. - Remove dummy test. R=thakis@chromium.org,felt@chromium.org,lgarron@chromium.org BUG=587694 ========== to ========== Fix latent bug in GetBuildTime(). - Change the generated_build_date.h to contain the full timestamp, it makes the code less brittle. - For official builds before 5:00am, it would generate a build time in the future. Fix this. - Improve the error message in BuildTime.InThePast. - Add test case to ensure the build is not more than 45 days old. - Remove dummy test. - Improve comments in base/BUILD.gn and base/base.gyp. R=thakis@chromium.org,felt@chromium.org,lgarron@chromium.org BUG=587694 ==========
Description was changed from ========== Fix latent bug in GetBuildTime(). - Change the generated_build_date.h to contain the full timestamp, it makes the code less brittle. - For official builds before 5:00am, it would generate a build time in the future. Fix this. - Improve the error message in BuildTime.InThePast. - Add test case to ensure the build is not more than 45 days old. - Remove dummy test. - Improve comments in base/BUILD.gn and base/base.gyp. R=thakis@chromium.org,felt@chromium.org,lgarron@chromium.org BUG=587694 ========== to ========== Fix latent bug in GetBuildTime(). - Change the generated_build_date.h to contain the full timestamp, it makes the code less brittle. - For official builds before 5:00am, it would generate a build time in the future. Fix this. - Improve the error message in BuildTime.InThePast. - Add test case to ensure the build is not more than 45 days old. - Remove dummy test. - Improve comments in base/BUILD.gn and base/base.gyp. - Add functional write_build_date_header.py --help. R=thakis@chromium.org,felt@chromium.org,lgarron@chromium.org BUG=587694 ==========
Please review.
Description was changed from ========== Fix latent bug in GetBuildTime(). - Change the generated_build_date.h to contain the full timestamp, it makes the code less brittle. - For official builds before 5:00am, it would generate a build time in the future. Fix this. - Improve the error message in BuildTime.InThePast. - Add test case to ensure the build is not more than 45 days old. - Remove dummy test. - Improve comments in base/BUILD.gn and base/base.gyp. - Add functional write_build_date_header.py --help. R=thakis@chromium.org,felt@chromium.org,lgarron@chromium.org BUG=587694 ========== to ========== Fix latent bug in GetBuildTime(). - Change the generated_build_date.h to contain the full timestamp, it makes the code less brittle. - For official builds before 5:00am, it would generate a build time in the future. Fix this. - Improve the error message in BuildTime.InThePast. - Add test case to ensure the build is not more than 45 days old. - Remove dummy test. - Improve comments in base/BUILD.gn and base/base.gyp. - Add functional write_build_date_header.py --help. R=thakis@chromium.org,felt@chromium.org,lgarron@chromium.org BUG=587694 ==========
maruel@chromium.org changed reviewers: + felt@chromium.org, lgarron@chromium.org
On 2016/03/30 21:19:47, M-A Ruel wrote: > Please review. Oops, I had meant to include felt@ and lgarron@ as reviewers.
Description was changed from ========== Fix latent bug in GetBuildTime(). - Change the generated_build_date.h to contain the full timestamp, it makes the code less brittle. - For official builds before 5:00am, it would generate a build time in the future. Fix this. - Improve the error message in BuildTime.InThePast. - Add test case to ensure the build is not more than 45 days old. - Remove dummy test. - Improve comments in base/BUILD.gn and base/base.gyp. - Add functional write_build_date_header.py --help. R=thakis@chromium.org,felt@chromium.org,lgarron@chromium.org BUG=587694 ========== to ========== Fix latent bug in GetBuildTime(). - Change the generated_build_date.h to contain the full timestamp, it makes the code less brittle. - For official builds before 5:00am, it would generate a build time in the future. Fix this. - Improve the error message in BuildTime.InThePast. - Add test case to ensure the build is not more than 45 days old. - Remove dummy test. - Improve comments in base/BUILD.gn and base/base.gyp. - Add functional write_build_date_header.py --help. This change builds upon work in https://codereview.chromium.org/1641413002. R=thakis@chromium.org,felt@chromium.org,lgarron@chromium.org BUG=587694 ==========
lgtm!
lgarron, can you review first? then i'll take a look.
On 2016/03/30 21:39:35, felt wrote: > lgarron, can you review first? then i'll take a look. ping
On 2016/03/31 at 22:01:24, maruel wrote: > On 2016/03/30 21:39:35, felt wrote: > > lgarron, can you review first? then i'll take a look. > > ping Hi! (I'm OOO, but let me take a look right now.) I'm glad that I asked for BuildTime.InThePast, since caught a real bug. :-) I left two comments, but the whole change LGTM.
On 2016/04/01 03:20:32, lgarron wrote: > On 2016/03/31 at 22:01:24, maruel wrote: > > On 2016/03/30 21:39:35, felt wrote: > > > lgarron, can you review first? then i'll take a look. > > > > ping > > Hi! (I'm OOO, but let me take a look right now.) > > I'm glad that I asked for BuildTime.InThePast, since caught a real bug. :-) > > I left two comments, but the whole change LGTM. You forgot to publish your comments. Please either (or all) append OOO to your alias, add an event to your calendar (preferred), put a OOO automated reply. Thanks
Generally LGTM. Not sure why you are using the weird char array and comparing character by character? Also, do the new doctests actually get run as part of the commit queue?
https://codereview.chromium.org/1846713002/diff/40001/build/write_build_date_... File build/write_build_date_header.py (right): https://codereview.chromium.org/1846713002/diff/40001/build/write_build_date_... build/write_build_date_header.py:73: if doctest.testmod()[0]: ^ mithro: see here for the doctest question
lgarron@chromium.org changed reviewers: + tansell@chromium.org
maruel@: It seems I didn't add/persist the proper calendar OOO. :-( Thanks for pointing out the alias change; I'll make sure to do that in the future if I'm gone for more than a few days. https://codereview.chromium.org/1846713002/diff/40001/build/write_build_date_... File build/write_build_date_header.py (right): https://codereview.chromium.org/1846713002/diff/40001/build/write_build_date_... build/write_build_date_header.py:29: >>> GetFirstSundayOfMonth(2016, 2) Any reason these shouldn't stay asserts? https://codereview.chromium.org/1846713002/diff/40001/build/write_build_date_... build/write_build_date_header.py:94: # invalidation to not happen exactly at midnight. Take the day before. Could you rephrase this to something like "Use the same calculation as the day before"? I had the code to quell my concern that this might round down only one day for those 5 hours (as opposed to rounding all the way to the beginning of the previous month).
https://codereview.chromium.org/1846713002/diff/40001/build/write_build_date_... File build/write_build_date_header.py (right): https://codereview.chromium.org/1846713002/diff/40001/build/write_build_date_... build/write_build_date_header.py:29: >>> GetFirstSundayOfMonth(2016, 2) On 2016/04/04 17:43:24, lgarron wrote: > Any reason these shouldn't stay asserts? because asserts will not print both values when it fails. doctest does. So I started with creating a function static_assert() but it felt more clunky. doctest is standard and intuitive. https://codereview.chromium.org/1846713002/diff/40001/build/write_build_date_... build/write_build_date_header.py:94: # invalidation to not happen exactly at midnight. Take the day before. On 2016/04/04 17:43:24, lgarron wrote: > Could you rephrase this to something like "Use the same calculation as the day > before"? > > I had the code to quell my concern that this might round down only one day for > those 5 hours (as opposed to rounding all the way to the beginning of the > previous month). Done.
The CQ bit was checked by maruel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lgarron@chromium.org, thakis@chromium.org, tansell@chromium.org Link to the patchset: https://codereview.chromium.org/1846713002/#ps60001 (title: "Improved comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1846713002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1846713002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix latent bug in GetBuildTime(). - Change the generated_build_date.h to contain the full timestamp, it makes the code less brittle. - For official builds before 5:00am, it would generate a build time in the future. Fix this. - Improve the error message in BuildTime.InThePast. - Add test case to ensure the build is not more than 45 days old. - Remove dummy test. - Improve comments in base/BUILD.gn and base/base.gyp. - Add functional write_build_date_header.py --help. This change builds upon work in https://codereview.chromium.org/1641413002. R=thakis@chromium.org,felt@chromium.org,lgarron@chromium.org BUG=587694 ========== to ========== Fix latent bug in GetBuildTime(). - Change the generated_build_date.h to contain the full timestamp, it makes the code less brittle. - For official builds before 5:00am, it would generate a build time in the future. Fix this. - Improve the error message in BuildTime.InThePast. - Add test case to ensure the build is not more than 45 days old. - Remove dummy test. - Improve comments in base/BUILD.gn and base/base.gyp. - Add functional write_build_date_header.py --help. This change builds upon work in https://codereview.chromium.org/1641413002. R=thakis@chromium.org,felt@chromium.org,lgarron@chromium.org BUG=587694 Committed: https://crrev.com/1c9b02233631b0ba9b096d79bf5b8d71a370dbb0 Cr-Commit-Position: refs/heads/master@{#384991} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1c9b02233631b0ba9b096d79bf5b8d71a370dbb0 Cr-Commit-Position: refs/heads/master@{#384991}
Message was sent while issue was closed.
On 2016/04/04 20:23:13, commit-bot: I haz the power wrote: > Patchset 4 (id:??) landed as > https://crrev.com/1c9b02233631b0ba9b096d79bf5b8d71a370dbb0 > Cr-Commit-Position: refs/heads/master@{#384991} I get a FAILED: python ../../build/write_build_date_header.py clang_x64/gen/base/generated_build_date.h default Traceback (most recent call last): File "../../build/write_build_date_header.py", line 118, in <module> sys.exit(main()) File "../../build/write_build_date_header.py", line 97, in main now = now - datetime.timedelta(day=1) TypeError: 'day' is an invalid keyword argument for this function Is there anything special I need to do to resolve this?
Message was sent while issue was closed.
On 2016/04/05 00:08:28, vmpstr wrote: > On 2016/04/04 20:23:13, commit-bot: I haz the power wrote: > > Patchset 4 (id:??) landed as > > https://crrev.com/1c9b02233631b0ba9b096d79bf5b8d71a370dbb0 > > Cr-Commit-Position: refs/heads/master@{#384991} > > I get a > > FAILED: python ../../build/write_build_date_header.py > clang_x64/gen/base/generated_build_date.h default > Traceback (most recent call last): > File "../../build/write_build_date_header.py", line 118, in <module> > sys.exit(main()) > File "../../build/write_build_date_header.py", line 97, in main > now = now - datetime.timedelta(day=1) > TypeError: 'day' is an invalid keyword argument for this function > > > Is there anything special I need to do to resolve this? https://chromium.googlesource.com/chromium/src/+/031aebddd2c595f9bcbf9c11457b... https://codereview.chromium.org/1862503002 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
