Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2068073002:
Provide v8 dependency on tracing. (Closed)
|
Created:
4 years, 6 months ago by Mircea Trofin Modified:
4 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionProvide v8 dependency on tracing.
Related to https://codereview.chromium.org/2061933002/. When
that CL lands, the variable introduced by the present CL would provide
the root for the path to the tracing includes, in the context of the
chromium build.
BUG=
Committed: https://crrev.com/6d4418ff859750f445af6e4612a5665ae2f84837
Cr-Commit-Position: refs/heads/master@{#400355}
Patch Set 1 #
Total comments: 3
Patch Set 2 : depth #
Total comments: 1
Messages
Total messages: 29 (12 generated)
Description was changed from ========== Provide v8 dependency on tracing. BUG= ========== to ========== Provide v8 dependency on tracing. Related to https://codereview.chromium.org/2061933002/ lands BUG= ==========
mtrofin@chromium.org changed reviewers: + fmeawad@chromium.org, machenbach@chromium.org
Description was changed from ========== Provide v8 dependency on tracing. Related to https://codereview.chromium.org/2061933002/ lands BUG= ========== to ========== Provide v8 dependency on tracing. Related to https://codereview.chromium.org/2061933002/. When that CL lands, the variable introduced by the present CL would provide the root for the path to the tracing includes, in the context of the chromium build. BUG= ==========
gentle reminder
The theory behind the approach seems legit. LGTM from my side.
The CQ bit was checked by mtrofin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2068073002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
lgtm. https://codereview.chromium.org/2068073002/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/2068073002/diff/1/build/common.gypi#newcode1287 build/common.gypi:1287: 'v8_tracing_include_dir': '..', Does this work with '<(DEPTH)' ? I think that resolves to the same place and makes the intent a little clearer.
https://codereview.chromium.org/2068073002/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/2068073002/diff/1/build/common.gypi#newcode1287 build/common.gypi:1287: 'v8_tracing_include_dir': '..', On 2016/06/16 22:42:25, Dirk Pranke wrote: > Does this work with '<(DEPTH)' ? I think that resolves to the same place and > makes the intent a little clearer. Yes it does - thanks for the suggestion!
The CQ bit was checked by mtrofin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, fmeawad@chromium.org Link to the patchset: https://codereview.chromium.org/2068073002/#ps20001 (title: "depth")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2068073002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by mtrofin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2068073002/20001
Message was sent while issue was closed.
Description was changed from ========== Provide v8 dependency on tracing. Related to https://codereview.chromium.org/2061933002/. When that CL lands, the variable introduced by the present CL would provide the root for the path to the tracing includes, in the context of the chromium build. BUG= ========== to ========== Provide v8 dependency on tracing. Related to https://codereview.chromium.org/2061933002/. When that CL lands, the variable introduced by the present CL would provide the root for the path to the tracing includes, in the context of the chromium build. BUG= ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Provide v8 dependency on tracing. Related to https://codereview.chromium.org/2061933002/. When that CL lands, the variable introduced by the present CL would provide the root for the path to the tracing includes, in the context of the chromium build. BUG= ========== to ========== Provide v8 dependency on tracing. Related to https://codereview.chromium.org/2061933002/. When that CL lands, the variable introduced by the present CL would provide the root for the path to the tracing includes, in the context of the chromium build. BUG= Committed: https://crrev.com/6d4418ff859750f445af6e4612a5665ae2f84837 Cr-Commit-Position: refs/heads/master@{#400355} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/6d4418ff859750f445af6e4612a5665ae2f84837 Cr-Commit-Position: refs/heads/master@{#400355}
Message was sent while issue was closed.
https://codereview.chromium.org/2068073002/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/2068073002/diff/1/build/common.gypi#newcode1287 build/common.gypi:1287: 'v8_tracing_include_dir': '..', On 2016/06/16 22:49:46, Mircea Trofin wrote: > On 2016/06/16 22:42:25, Dirk Pranke wrote: > > Does this work with '<(DEPTH)' ? I think that resolves to the same place and > > makes the intent a little clearer. > > Yes it does - thanks for the suggestion! I wonder why we need a chromium side variable at all when using <(DEPTH)? Can we not just inline <(DEPTH) in the include path on the v8 side? In a chromium context it would resolve to "src", while in v8 stand-alone to "v8", wouldn't it? https://codereview.chromium.org/2068073002/diff/20001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/2068073002/diff/20001/build/common.gypi#newco... build/common.gypi:1287: 'v8_tracing_include_dir': '<(DEPTH)', nit: It might be minimally confusing that there's no empty line between the lines and the one above has a comment. It looks like the things belong together but they don't.
Message was sent while issue was closed.
On 2016/06/17 06:41:04, Michael Achenbach wrote: > https://codereview.chromium.org/2068073002/diff/1/build/common.gypi > File build/common.gypi (right): > > https://codereview.chromium.org/2068073002/diff/1/build/common.gypi#newcode1287 > build/common.gypi:1287: 'v8_tracing_include_dir': '..', > On 2016/06/16 22:49:46, Mircea Trofin wrote: > > On 2016/06/16 22:42:25, Dirk Pranke wrote: > > > Does this work with '<(DEPTH)' ? I think that resolves to the same place and > > > makes the intent a little clearer. > > > > Yes it does - thanks for the suggestion! > > I wonder why we need a chromium side variable at all when using <(DEPTH)? Can we > not just inline <(DEPTH) in the include path on the v8 side? In a chromium > context it would resolve to "src", while in v8 stand-alone to "v8", wouldn't it? I agree that <(DEPTH) alone might be sufficient; I wasn't sure how things were laid out in the standalone build.
Message was sent while issue was closed.
On 2016/06/17 15:56:06, Dirk Pranke wrote: > On 2016/06/17 06:41:04, Michael Achenbach wrote: > > https://codereview.chromium.org/2068073002/diff/1/build/common.gypi > > File build/common.gypi (right): > > > > > https://codereview.chromium.org/2068073002/diff/1/build/common.gypi#newcode1287 > > build/common.gypi:1287: 'v8_tracing_include_dir': '..', > > On 2016/06/16 22:49:46, Mircea Trofin wrote: > > > On 2016/06/16 22:42:25, Dirk Pranke wrote: > > > > Does this work with '<(DEPTH)' ? I think that resolves to the same place > and > > > > makes the intent a little clearer. > > > > > > Yes it does - thanks for the suggestion! > > > > I wonder why we need a chromium side variable at all when using <(DEPTH)? Can > we > > not just inline <(DEPTH) in the include path on the v8 side? In a chromium > > context it would resolve to "src", while in v8 stand-alone to "v8", wouldn't > it? > > I agree that <(DEPTH) alone might be sufficient; I wasn't sure how things were > laid out > in the standalone build. We verified on the https://codereview.chromium.org/2061933002/ side it's sufficient, indeed. I'll do a revert of this CL shortly. Thanks!
Message was sent while issue was closed.
Any reason why we don't revert this right away? I'll do this now so that it doesn't fall into the cracks.
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2085883002/ by machenbach@chromium.org. The reason for reverting is: Feature not needed.. |
