Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2061933002:
[build] Provide tracing dependency via variable (Closed)
|
Created:
4 years, 6 months ago by Mircea Trofin Modified:
4 years, 6 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[build] Provide tracing dependency via variable
This avoids introducing an include path outside v8. Instead, the path
to where tracing includes are located is provided via a variable.
Please refer to https://codereview.chromium.org/2068073002.
Once the v8 change makes its way to chromium, we can remove the
TODO in this current change.
BUG=
Committed: https://crrev.com/7442f537bc4c460626be6d597be222ef368d28f7
Cr-Commit-Position: refs/heads/master@{#37049}
Patch Set 1 : [build] Remove unnecessary include out of root v8 #
Total comments: 2
Patch Set 2 : patch #
Total comments: 2
Patch Set 3 : patch #Patch Set 4 : depths #
Total comments: 1
Patch Set 5 : using depth #Messages
Total messages: 28 (10 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== [build] Remove unnecessary include out of root v8 BUG= ========== to ========== [build] Remove unnecessary include out of root v8 The include is for anything outside the v8 root. It appears to be unnecessary. In addition, it upsets IDEs like CLion, that rely on cmake. CLion starts indexing everything outside the v8 folder as result, which consumes time unnecessarily. BUG= ==========
mtrofin@chromium.org changed reviewers: + machenbach@chromium.org
machenbach@chromium.org changed reviewers: + dsinclair@chromium.org, fmeawad@chromium.org, yangguo@chromium.org
machenbach@chromium.org changed reviewers: + dsinclair@chromium.org, fmeawad@chromium.org, yangguo@chromium.org
Adding author and reviewers of the CL that added the lines: https://codereview.chromium.org/988893003 You might not see the impact of your change on any v8 cq bot as this affects only chromium and gyp. Additional chromium bots that use gyp can be run manually, I try to grab one or two...
Adding author and reviewers of the CL that added the lines: https://codereview.chromium.org/988893003 You might not see the impact of your change on any v8 cq bot as this affects only chromium and gyp. Additional chromium bots that use gyp can be run manually, I try to grab one or two...
https://codereview.chromium.org/2061933002/diff/20001/src/v8.gyp File src/v8.gyp (left): https://codereview.chromium.org/2061933002/diff/20001/src/v8.gyp#oldcode391 src/v8.gyp:391: '../..', This line was added to allow v8, when built inside chromium to be able to access the chromium version of the file mentioned in the comment. Removing it would break the chromium build AFAIK.
https://codereview.chromium.org/2061933002/diff/20001/src/v8.gyp File src/v8.gyp (left): https://codereview.chromium.org/2061933002/diff/20001/src/v8.gyp#oldcode391 src/v8.gyp:391: '../..', On 2016/06/14 18:26:33, fmeawad wrote: > This line was added to allow v8, when built inside chromium to be able to access > the chromium version of the file mentioned in the comment. Removing it would > break the chromium build AFAIK. It was done this way because v8 inside chrome does not have any dependencies, and to avoid cyclic dependencies.
Also the two chromium trybots show that it'll break like that. But feel free to find a different solution. I think in GN, we solved this in a different way.
Description was changed from ========== [build] Remove unnecessary include out of root v8 The include is for anything outside the v8 root. It appears to be unnecessary. In addition, it upsets IDEs like CLion, that rely on cmake. CLion starts indexing everything outside the v8 folder as result, which consumes time unnecessarily. BUG= ========== to ========== [build] Provide tracing dependency via variable This avoids introducing an include path outside v8. Instead, the path to where tracing includes are located is provided via a variable. Please refer to https://codereview.chromium.org/2068073002. Once the v8 change makes its way to chromium, we can remove the TODO in this current change. BUG= ==========
https://codereview.chromium.org/2061933002/diff/40001/src/v8.gyp File src/v8.gyp (right): https://codereview.chromium.org/2061933002/diff/40001/src/v8.gyp#newcode392 src/v8.gyp:392: '<(v8_tracing_include_dir)', Won't that fail if you declare just in the standalone.gypi? Unless you define it in chromium first...
https://codereview.chromium.org/2061933002/diff/40001/src/v8.gyp File src/v8.gyp (right): https://codereview.chromium.org/2061933002/diff/40001/src/v8.gyp#newcode392 src/v8.gyp:392: '<(v8_tracing_include_dir)', On 2016/06/15 06:42:43, Michael Achenbach wrote: > Won't that fail if you declare just in the standalone.gypi? Unless you define it > in chromium first... No, I left the ../.. Under the Todo. I like your idea better though - going chrome first. Then there's no need for the Todo here, nor the ../..
PTAL - the chromium side of things just landed, and the patch on the v8 side appears to work as expected.
lgtm - but please check if my suggestion below works too. https://codereview.chromium.org/2061933002/diff/80001/src/v8.gyp File src/v8.gyp (right): https://codereview.chromium.org/2061933002/diff/80001/src/v8.gyp#newcode390 src/v8.gyp:390: '<(v8_tracing_include_dir)', See also comment on the other CL. Why not just use <(DEPTH) here? And no variable at all.
On 2016/06/17 06:43:32, Michael Achenbach wrote: > lgtm - but please check if my suggestion below works too. > > https://codereview.chromium.org/2061933002/diff/80001/src/v8.gyp > File src/v8.gyp (right): > > https://codereview.chromium.org/2061933002/diff/80001/src/v8.gyp#newcode390 > src/v8.gyp:390: '<(v8_tracing_include_dir)', > See also comment on the other CL. Why not just use <(DEPTH) here? And no > variable at all. It appears to work! so <(DEPTH) refers to the start of the enlistment? This means I can revert the Chromium side, too. Simpler! Thanks!
The CQ bit was checked by mtrofin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/2061933002/#ps100001 (title: "using depth")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2061933002/100001
Message was sent while issue was closed.
Description was changed from ========== [build] Provide tracing dependency via variable This avoids introducing an include path outside v8. Instead, the path to where tracing includes are located is provided via a variable. Please refer to https://codereview.chromium.org/2068073002. Once the v8 change makes its way to chromium, we can remove the TODO in this current change. BUG= ========== to ========== [build] Provide tracing dependency via variable This avoids introducing an include path outside v8. Instead, the path to where tracing includes are located is provided via a variable. Please refer to https://codereview.chromium.org/2068073002. Once the v8 change makes its way to chromium, we can remove the TODO in this current change. BUG= ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [build] Provide tracing dependency via variable This avoids introducing an include path outside v8. Instead, the path to where tracing includes are located is provided via a variable. Please refer to https://codereview.chromium.org/2068073002. Once the v8 change makes its way to chromium, we can remove the TODO in this current change. BUG= ========== to ========== [build] Provide tracing dependency via variable This avoids introducing an include path outside v8. Instead, the path to where tracing includes are located is provided via a variable. Please refer to https://codereview.chromium.org/2068073002. Once the v8 change makes its way to chromium, we can remove the TODO in this current change. BUG= Committed: https://crrev.com/7442f537bc4c460626be6d597be222ef368d28f7 Cr-Commit-Position: refs/heads/master@{#37049} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7442f537bc4c460626be6d597be222ef368d28f7 Cr-Commit-Position: refs/heads/master@{#37049}
Message was sent while issue was closed.
Did you check with a chromium gyp bot? Now I can't trigger one because the CL landed... we might see on our waterfall though...
Message was sent while issue was closed.
On 2016/06/17 07:27:09, Michael Achenbach wrote: > Did you check with a chromium gyp bot? Now I can't trigger one because the CL > landed... we might see on our waterfall though... Never mind. Now I saw the mac bot. Everything good. Guess the chromium CL can be reverted... |
