Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2246203004:
Add a copy of gyp_flag_compare from Chromium to WebRTC's webrtc/tools. (Closed)
|
Created:
4 years, 4 months ago by ehmaldonado_webrtc Modified:
4 years, 4 months ago Reviewers:
kjellander_webrtc CC:
webrtc-reviews_webrtc.org Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd a copy of gyp_flag_compare from Chromium to WebRTC's webrtc/tools.
This should help spot any differences between GN and GYP.
BUG=webrtc:5949
NOTRY=True
Committed: https://crrev.com/94b9199fb1ba7b2ec2fb4ef7a013beca5b7c97d4
Cr-Commit-Position: refs/heads/master@{#13840}
Patch Set 1 : Current Chromium script. #Patch Set 2 : V8's script. #Patch Set 3 : Changes for WebRTC. #
Total comments: 1
Patch Set 4 : Port from Chromium's script. #
Total comments: 1
Patch Set 5 : Update comments. #
Total comments: 4
Patch Set 6 : Addressed comments. #Messages
Total messages: 33 (16 generated)
Description was changed from ========== Add a copy of gyp_flag_compare from v8 to WebRTC's webrtc/tools. BUG= ========== to ========== Add a copy of gyp_flag_compare from v8 to WebRTC's webrtc/tools. This should help spot any differences between GN and GYP. BUG=webrtc:5949 NOTRY=True ==========
ehmaldonado@webrtc.org changed reviewers: + kjellander@webrtc.org
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
I was thinking of something like this.
Oh, missed this one. Is this an unmodified copy of V8's version? If not, can you check in V8 pristine version as PS#1 and then do your changes in PS#2? Also: can you add a link to the CL where V8's version was reviewed to this CL description? I just think it's useful to know how much the script has diverged from the original version in Chromium.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #3 (id:120001) has been deleted
Description was changed from ========== Add a copy of gyp_flag_compare from v8 to WebRTC's webrtc/tools. This should help spot any differences between GN and GYP. BUG=webrtc:5949 NOTRY=True ========== to ========== Add a copy of gyp_flag_compare from v8 to WebRTC's webrtc/tools. This should help spot any differences between GN and GYP. CL where the script was added to v8: https://codereview.chromium.org/1988163002 Subsequent changes: https://codereview.chromium.org/2141903002 BUG=webrtc:5949 NOTRY=True ==========
On 2016/08/19 06:51:04, kjellander_webrtc wrote: > Oh, missed this one. > Is this an unmodified copy of V8's version? If not, can you check in V8 pristine > version as PS#1 and then do your changes in PS#2? > > Also: can you add a link to the CL where V8's version was reviewed to this CL > description? I just think it's useful to know how much the script has diverged > from the original version in Chromium. PTAL
Awesome, so much easier to review! lgtm
The CQ bit was checked by ehmaldonado@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2246203004/diff/140001/tools/gyp_flag_compare.py File tools/gyp_flag_compare.py (right): https://codereview.webrtc.org/2246203004/diff/140001/tools/gyp_flag_compare.p... tools/gyp_flag_compare.py:27: g_total_differences = 0 Add the following comment to this line to ignore the lint check: # pylint: disable=global-statement I think that's fine since it's beneficial to not diverge too much from the Chromium/V8 script.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7552)
Description was changed from ========== Add a copy of gyp_flag_compare from v8 to WebRTC's webrtc/tools. This should help spot any differences between GN and GYP. CL where the script was added to v8: https://codereview.chromium.org/1988163002 Subsequent changes: https://codereview.chromium.org/2141903002 BUG=webrtc:5949 NOTRY=True ========== to ========== Add a copy of gyp_flag_compare from Chromium to WebRTC's webrtc/tools. This should help spot any differences between GN and GYP. BUG=webrtc:5949 NOTRY=True ==========
PTAL. The pristine chromium script is the PS#1
Now it's a bit confusing. It's not clear what is expected from the script (I think it requires two directories with GYP and GN already existing, but it doesn't make that clear nor gives a good error message if that's not true). https://codereview.webrtc.org/2246203004/diff/160001/tools/gyp_flag_compare.py File tools/gyp_flag_compare.py (right): https://codereview.webrtc.org/2246203004/diff/160001/tools/gyp_flag_compare.p... tools/gyp_flag_compare.py:14: This script is best used interactively. Here's a recommended setup: This part is no longer true, right? I think V8's version script was mainly modified to better adapt to being run on a bot (where GYP and GN had already executed), while Chromium's version was running GYP and GN every time it executed (which could be nice, but it slow to iterate with). The Chromium one seems to be more targeted to running interactively like described here as well. If that's what we want, we should keep the ConfigureBuild method (but update it to use our GYP script in webrtc/build/gyp_webrtc.py). I'll let you decide which way you think the script is most useful for us right now.
Patchset #5 (id:180001) has been deleted
Patchset #5 (id:110002) has been deleted
On 2016/08/19 13:09:09, kjellander_webrtc wrote: > Now it's a bit confusing. It's not clear what is expected from the script (I > think it requires two directories with GYP and GN already existing, but it > doesn't make that clear nor gives a good error message if that's not true). > > https://codereview.webrtc.org/2246203004/diff/160001/tools/gyp_flag_compare.py > File tools/gyp_flag_compare.py (right): > > https://codereview.webrtc.org/2246203004/diff/160001/tools/gyp_flag_compare.p... > tools/gyp_flag_compare.py:14: This script is best used interactively. Here's a > recommended setup: > This part is no longer true, right? I think V8's version script was mainly > modified to better adapt to being run on a bot (where GYP and GN had already > executed), while Chromium's version was running GYP and GN every time it > executed (which could be nice, but it slow to iterate with). > > The Chromium one seems to be more targeted to running interactively like > described here as well. If that's what we want, we should keep the > ConfigureBuild method (but update it to use our GYP script in > webrtc/build/gyp_webrtc.py). I'll let you decide which way you think the script > is most useful for us right now. The script now mimics the V8's script behavior when called from the command line, which I find most useful right now. It also supports interactivity, which I don't think gets in the way. PTAL
Nice to have both approaches covered in our script (at least for now). Just a few more comments, then we're done. https://codereview.webrtc.org/2246203004/diff/210001/tools/gyp_flag_compare.py File tools/gyp_flag_compare.py (right): https://codereview.webrtc.org/2246203004/diff/210001/tools/gyp_flag_compare.p... tools/gyp_flag_compare.py:20: When the GN and GYP targets differ, it should be called invoked as follows: Rephrase to: When the GN and GYP target names differ, it should be called invoked as follows: https://codereview.webrtc.org/2246203004/diff/210001/tools/gyp_flag_compare.p... tools/gyp_flag_compare.py:24: This script can be used interactively. Here's a recommended setup: I suggest: This script can also be used interactively. Then ConfigureBuild can optionally be used to generate ninja files with GYP and GN. Here's an example setup: https://codereview.webrtc.org/2246203004/diff/210001/tools/gyp_flag_compare.p... tools/gyp_flag_compare.py:25: $ PYTHONPATH=tools python This example assumes the current working directory is the project root (in order for webrtc/build/gyp_webrtc to be a valid path). Then that assumption should be documented here. https://codereview.webrtc.org/2246203004/diff/210001/tools/gyp_flag_compare.p... tools/gyp_flag_compare.py:90: Run('python webrtc/build/gyp_webrtc -Gconfig=Release %s' % gyp_defines) gyp_webrtc -> gyp_webrtc.py since the former is just a compatibility wrapper script.
On 2016/08/22 06:34:30, kjellander_webrtc wrote: > Nice to have both approaches covered in our script (at least for now). Just a > few more comments, then we're done. > > https://codereview.webrtc.org/2246203004/diff/210001/tools/gyp_flag_compare.py > File tools/gyp_flag_compare.py (right): > > https://codereview.webrtc.org/2246203004/diff/210001/tools/gyp_flag_compare.p... > tools/gyp_flag_compare.py:20: When the GN and GYP targets differ, it should be > called invoked as follows: > Rephrase to: > When the GN and GYP target names differ, it should be called invoked as follows: > > https://codereview.webrtc.org/2246203004/diff/210001/tools/gyp_flag_compare.p... > tools/gyp_flag_compare.py:24: This script can be used interactively. Here's a > recommended setup: > I suggest: > This script can also be used interactively. Then ConfigureBuild can optionally > be used to generate ninja files with GYP and GN. > Here's an example setup: > > https://codereview.webrtc.org/2246203004/diff/210001/tools/gyp_flag_compare.p... > tools/gyp_flag_compare.py:25: $ PYTHONPATH=tools python > This example assumes the current working directory is the project root (in order > for webrtc/build/gyp_webrtc to be a valid path). > Then that assumption should be documented here. > > https://codereview.webrtc.org/2246203004/diff/210001/tools/gyp_flag_compare.p... > tools/gyp_flag_compare.py:90: Run('python webrtc/build/gyp_webrtc > -Gconfig=Release %s' % gyp_defines) > gyp_webrtc -> gyp_webrtc.py > since the former is just a compatibility wrapper script. PTAL
lgtm
The CQ bit was checked by ehmaldonado@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Add a copy of gyp_flag_compare from Chromium to WebRTC's webrtc/tools. This should help spot any differences between GN and GYP. BUG=webrtc:5949 NOTRY=True ========== to ========== Add a copy of gyp_flag_compare from Chromium to WebRTC's webrtc/tools. This should help spot any differences between GN and GYP. BUG=webrtc:5949 NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #6 (id:230001)
Message was sent while issue was closed.
Description was changed from ========== Add a copy of gyp_flag_compare from Chromium to WebRTC's webrtc/tools. This should help spot any differences between GN and GYP. BUG=webrtc:5949 NOTRY=True ========== to ========== Add a copy of gyp_flag_compare from Chromium to WebRTC's webrtc/tools. This should help spot any differences between GN and GYP. BUG=webrtc:5949 NOTRY=True Committed: https://crrev.com/94b9199fb1ba7b2ec2fb4ef7a013beca5b7c97d4 Cr-Commit-Position: refs/heads/master@{#13840} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/94b9199fb1ba7b2ec2fb4ef7a013beca5b7c97d4 Cr-Commit-Position: refs/heads/master@{#13840} |
