Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1692303004:
Update GN bootstrap build (Closed)
DescriptionUpdate GN bootstrap build
Bootstrap didn't work due to recent modifications in base.
Unfortunately some new dependencies are required on OSX
and that required adding generated_build_date.h to be
available separately for the bootstrap process.
Tested on Linux and OSX.
BUG=None
TEST=./tools/gn/bootstrap/bootstrap.py
R=brettw@chromium.org,tfarina@chromium.org
Committed: https://crrev.com/cda4afd7efda6e3034d58d9f375b01ff3f36d220
Cr-Commit-Position: refs/heads/master@{#376133}
Patch Set 1 #
Total comments: 1
Patch Set 2 : #Patch Set 3 : try to get cl not to recognize new file as copy (similarity=0) #Patch Set 4 : Another try with similarity=99 #Patch Set 5 : Use real BUILD_DATE #
Total comments: 2
Patch Set 6 : add description to generated_build_date.h #
Total comments: 1
Patch Set 7 : generate generated_build_date.h #Patch Set 8 : remove unnecessary imports #
Total comments: 3
Patch Set 9 : update comments #
Total comments: 2
Patch Set 10 : wording of comments #
Total comments: 4
Messages
Total messages: 27 (6 generated)
tfarina@chromium.org changed reviewers: + tfarina@chromium.org
https://codereview.chromium.org/1692303004/diff/1/tools/gn/bootstrap/base/gen... File tools/gn/bootstrap/base/generated_build_date.h (left): https://codereview.chromium.org/1692303004/diff/1/tools/gn/bootstrap/base/gen... tools/gn/bootstrap/base/generated_build_date.h:5: window.didRunAtDocumentIdleUnexpected = true; Could you tweak the similarity passed to git cl, so it interprets generated_build_date.h as a new file correctly? Btw. From which base change is this necessary?
On 2016/02/13 16:46:54, tfarina wrote:
> Btw. From which base change is this necessary?
base/trace_event/process_memory_{maps,totals}_dump_provider.cc were removed in
https://codereview.chromium.org/1629393003 (landed on 26 Jan)
Histograms were refactored in https://codereview.chromium.org/1425533011 (landed
on 1 Feb) and now they use base::SharedMemory and the OSX implementation has
some other dependencies and this causes most of the necessary changes.
generated_build_date.h is needed because base::SharedMemory on OSX currently is
under A/B testing and it uses base::FieldTrial and that uses
base::GetBuildTime().
A syntactically valid BUILD_DATE is required, otherwise a DCHECK in
base::GetBuildTime() would fail.
I did not want to fix a date because of possible future side-effects, now
bootstrap.py defines the real time.
https://codereview.chromium.org/1692303004/diff/80001/tools/gn/bootstrap/base... File tools/gn/bootstrap/base/generated_build_date.h (right): https://codereview.chromium.org/1692303004/diff/80001/tools/gn/bootstrap/base... tools/gn/bootstrap/base/generated_build_date.h:6: #error BUILD_DATE must be defined by bootstrap.py Why do we need this file? It is not clear to me, maybe it is to Brett, but I think for future readers, it would be good if you could explain why we need this pseudo generated file.
Description was changed from ========== Update GN bootstrap build Bootstrap didn't work due to recent modifications in base. Unfortunately some new dependencies are required on OSX and that required adding generated_build_date.h to be available separately for the bootstrap process. Tested on Linux and OSX. R=brettw@chromium.org BUG= ========== to ========== Update GN bootstrap build Bootstrap didn't work due to recent modifications in base. Unfortunately some new dependencies are required on OSX and that required adding generated_build_date.h to be available separately for the bootstrap process. Tested on Linux and OSX. BUG=None TEST=./tools/gn/bootstrap/bootstrap.py R=brettw@chromium.org,tfarina@chromium.org ==========
https://codereview.chromium.org/1692303004/diff/80001/tools/gn/bootstrap/base... File tools/gn/bootstrap/base/generated_build_date.h (right): https://codereview.chromium.org/1692303004/diff/80001/tools/gn/bootstrap/base... tools/gn/bootstrap/base/generated_build_date.h:6: #error BUILD_DATE must be defined by bootstrap.py On 2016/02/14 20:10:59, tfarina wrote: > Why do we need this file? It is not clear to me, maybe it is to Brett, but I > think for future readers, it would be good if you could explain why we need this > pseudo generated file. Done.
Can bootstrap.py just generate the file instead?
On 2016/02/15 23:55:02, brettw wrote: > Can bootstrap.py just generate the file instead? I was hoping that could be possible as well.
https://codereview.chromium.org/1692303004/diff/100001/tools/gn/bootstrap/bas... File tools/gn/bootstrap/base/generated_build_date.h (right): https://codereview.chromium.org/1692303004/diff/100001/tools/gn/bootstrap/bas... tools/gn/bootstrap/base/generated_build_date.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Btw, how come build_date.cc is seeing this file in tools/gn/bootstrap? Is tools/gn/bootstrap being included in -I path? If so, could you show me where? Otherwise, I don't see how is is possible be working. Perhaps paste the command line that bootstrap is using to compile it, just for us to see what it is doing.
On 2016/02/16 14:47:05, tfarina wrote: > https://codereview.chromium.org/1692303004/diff/100001/tools/gn/bootstrap/bas... > File tools/gn/bootstrap/base/generated_build_date.h (right): > > https://codereview.chromium.org/1692303004/diff/100001/tools/gn/bootstrap/bas... > tools/gn/bootstrap/base/generated_build_date.h:1: // Copyright 2016 The Chromium > Authors. All rights reserved. > Btw, how come build_date.cc is seeing this file in tools/gn/bootstrap? Is > tools/gn/bootstrap being included in -I path? If so, could you show me where? > Otherwise, I don't see how is is possible be working. Perhaps paste the command > line that bootstrap is using to compile it, just for us to see what it is doing. BOOTSTRAP_DIR was included in include dirs: include_dirs = [BOOTSTRAP_DIR, SRC_ROOT] Now that file is auto-generated. Do you think it is ok to call //build/write_build_date_header.py from bootstrap.py? (See patch set 8 for this approach)
I think this is in a shape good to go now. Thanks for going through the changes with us. Lets wait for Brett to see what he thinks. LG from my side now. I think we already require //build for the standalone build iirc. https://codereview.chromium.org/1692303004/diff/140001/tools/gn/bootstrap/boo... File tools/gn/bootstrap/bootstrap.py (right): https://codereview.chromium.org/1692303004/diff/140001/tools/gn/bootstrap/boo... tools/gn/bootstrap/bootstrap.py:125: os.path.join(SRC_ROOT, 'build', 'write_build_date_header.py'), I think that is a better approach. But that is just my opinion and Brett might have other. Thanks for showing us this way.
https://codereview.chromium.org/1692303004/diff/140001/tools/gn/bootstrap/boo... File tools/gn/bootstrap/bootstrap.py (right): https://codereview.chromium.org/1692303004/diff/140001/tools/gn/bootstrap/boo... tools/gn/bootstrap/bootstrap.py:121: if is_mac: I think this should be always run, so you can just remove this line and re-indent the rest. If I'm wrong, there should be a comment about why this only applies on Mac. https://codereview.chromium.org/1692303004/diff/140001/tools/gn/bootstrap/boo... tools/gn/bootstrap/bootstrap.py:147: cflags.extend(['-DNO_TCMALLOC']) I don't see anything in the CL description about why you changed this. Is something broken if you don't do this?
On 2016/02/16 21:01:48, brettw wrote: > https://codereview.chromium.org/1692303004/diff/140001/tools/gn/bootstrap/boo... > File tools/gn/bootstrap/bootstrap.py (right): > > https://codereview.chromium.org/1692303004/diff/140001/tools/gn/bootstrap/boo... > tools/gn/bootstrap/bootstrap.py:121: if is_mac: > I think this should be always run, so you can just remove this line and > re-indent the rest. If I'm wrong, there should be a comment about why this only > applies on Mac. //base/build_time.cc is only needed for mac targets (needed because of base::SharedMemory implementation on Mac) > > https://codereview.chromium.org/1692303004/diff/140001/tools/gn/bootstrap/boo... > tools/gn/bootstrap/bootstrap.py:147: cflags.extend(['-DNO_TCMALLOC']) > I don't see anything in the CL description about why you changed this. Is > something broken if you don't do this? Without this flag there would be linker errors originating from //base/allocator/allocator_extension.cc Wrote comments in bootstrap.py for both of these
https://codereview.chromium.org/1692303004/diff/160001/tools/gn/bootstrap/boo... File tools/gn/bootstrap/bootstrap.py (right): https://codereview.chromium.org/1692303004/diff/160001/tools/gn/bootstrap/boo... tools/gn/bootstrap/bootstrap.py:123: # That file is only included for Mac builds Please, end this sentence with a period. Also I would add a comma after generated_build_date.h and continue the phrase with 'and this file...'. https://codereview.chromium.org/1692303004/diff/160001/tools/gn/bootstrap/boo... tools/gn/bootstrap/bootstrap.py:149: # originating from //base/allocator/allocator_extension.cc Or you could rewrite this as: //base/allocator/allocator_extension.cc needs this macro defined, otherwise there would be link errors.
LGTM with Thiago's suggested comment fix.
The CQ bit was checked by ngg@ngg.hu
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1692303004/#ps180001 (title: "wording of comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1692303004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1692303004/180001
Message was sent while issue was closed.
Description was changed from ========== Update GN bootstrap build Bootstrap didn't work due to recent modifications in base. Unfortunately some new dependencies are required on OSX and that required adding generated_build_date.h to be available separately for the bootstrap process. Tested on Linux and OSX. BUG=None TEST=./tools/gn/bootstrap/bootstrap.py R=brettw@chromium.org,tfarina@chromium.org ========== to ========== Update GN bootstrap build Bootstrap didn't work due to recent modifications in base. Unfortunately some new dependencies are required on OSX and that required adding generated_build_date.h to be available separately for the bootstrap process. Tested on Linux and OSX. BUG=None TEST=./tools/gn/bootstrap/bootstrap.py R=brettw@chromium.org,tfarina@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Update GN bootstrap build Bootstrap didn't work due to recent modifications in base. Unfortunately some new dependencies are required on OSX and that required adding generated_build_date.h to be available separately for the bootstrap process. Tested on Linux and OSX. BUG=None TEST=./tools/gn/bootstrap/bootstrap.py R=brettw@chromium.org,tfarina@chromium.org ========== to ========== Update GN bootstrap build Bootstrap didn't work due to recent modifications in base. Unfortunately some new dependencies are required on OSX and that required adding generated_build_date.h to be available separately for the bootstrap process. Tested on Linux and OSX. BUG=None TEST=./tools/gn/bootstrap/bootstrap.py R=brettw@chromium.org,tfarina@chromium.org Committed: https://crrev.com/cda4afd7efda6e3034d58d9f375b01ff3f36d220 Cr-Commit-Position: refs/heads/master@{#376133} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/cda4afd7efda6e3034d58d9f375b01ff3f36d220 Cr-Commit-Position: refs/heads/master@{#376133}
Message was sent while issue was closed.
https://codereview.chromium.org/1692303004/diff/180001/tools/gn/bootstrap/boo... File tools/gn/bootstrap/bootstrap.py (right): https://codereview.chromium.org/1692303004/diff/180001/tools/gn/bootstrap/boo... tools/gn/bootstrap/bootstrap.py:121: if is_mac: Wait, why is this only for Mac? It is not included conditionaly through #if defined(OS_MAC) in base/build_time.cc. https://chromium.googlesource.com/chromium/src/+/master/base/build_time.cc#8
Message was sent while issue was closed.
https://codereview.chromium.org/1692303004/diff/180001/tools/gn/bootstrap/boo... File tools/gn/bootstrap/bootstrap.py (right): https://codereview.chromium.org/1692303004/diff/180001/tools/gn/bootstrap/boo... tools/gn/bootstrap/bootstrap.py:121: if is_mac: On 2016/02/18 22:47:54, tfarina wrote: > Wait, why is this only for Mac? It is not included conditionaly through #if > defined(OS_MAC) in base/build_time.cc. > https://chromium.googlesource.com/chromium/src/+/master/base/build_time.cc#8 Because build_time.cc is not compiled for non-mac targets (see below at line 388)
Message was sent while issue was closed.
https://codereview.chromium.org/1692303004/diff/180001/tools/gn/bootstrap/boo... File tools/gn/bootstrap/bootstrap.py (right): https://codereview.chromium.org/1692303004/diff/180001/tools/gn/bootstrap/boo... tools/gn/bootstrap/bootstrap.py:121: if is_mac: On 2016/02/18 23:31:48, NGG wrote: > On 2016/02/18 22:47:54, tfarina wrote: > > Wait, why is this only for Mac? It is not included conditionaly through #if > > defined(OS_MAC) in base/build_time.cc. > > https://chromium.googlesource.com/chromium/src/+/master/base/build_time.cc#8 > > Because build_time.cc is not compiled for non-mac targets (see below at line > 388) This seems wrong. Why did you add build_time.cc only for Mac? It's unconditionally compiled in Chrome.
Message was sent while issue was closed.
https://codereview.chromium.org/1692303004/diff/180001/tools/gn/bootstrap/boo... File tools/gn/bootstrap/bootstrap.py (right): https://codereview.chromium.org/1692303004/diff/180001/tools/gn/bootstrap/boo... tools/gn/bootstrap/bootstrap.py:121: if is_mac: On 2016/02/19 22:13:48, brettw wrote: > On 2016/02/18 23:31:48, NGG wrote: > > On 2016/02/18 22:47:54, tfarina wrote: > > > Wait, why is this only for Mac? It is not included conditionaly through #if > > > defined(OS_MAC) in base/build_time.cc. > > > https://chromium.googlesource.com/chromium/src/+/master/base/build_time.cc#8 > > > > Because build_time.cc is not compiled for non-mac targets (see below at line > > 388) > > This seems wrong. Why did you add build_time.cc only for Mac? It's > unconditionally compiled in Chrome. It's only needed on Mac because //base/memory/shared_memory_mac.cc includes //base/metrics/field_trial.h and that has additional dependencies. It's not needed on Linux for compiling GN. |
