Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1988163002:
[gn] Port gyp/gn comparison script (Closed)
|
Created:
4 years, 7 months ago by Michael Achenbach Modified:
4 years, 7 months ago Reviewers:
vogelheim CC:
v8-reviews_googlegroups.com, jochen (gone - plz use gerrit) Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[gn] Port gyp/gn comparison script
BUG=chromium:474921
LOG=n
NOTRY=true
Committed: https://crrev.com/5e1c87dd9553a0ed3709caf706be300ab8c9b21a
Cr-Commit-Position: refs/heads/master@{#36356}
Patch Set 1 : Original unmodified script #Patch Set 2 : Modifications #
Total comments: 8
Messages
Total messages: 12 (6 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== [gn] Port gyp/gn comparison script BUG= ========== to ========== [gn] Port gyp/gn comparison script BUG=chromium:474921 LOG=n NOTRY=true ==========
machenbach@chromium.org changed reviewers: + vogelheim@chromium.org
PTAL. This ports the script from here: https://code.google.com/p/chromium/codesearch#chromium/src/tools/gn/bin/gyp_f... The original didn't change in a while, therefore forking it makes things easier, instead of trying to modify it upstream and pulling it as a deps, which would require to pull all gn sources atm. Some parts of this script are a bit ad hoc, e.g. cmd-line parameter passing, but I don't want to improve this much, given the temporary nature of the script. I added two patch sets, one with the unmodified original and one with my modifications, to ease review. https://codereview.chromium.org/1988163002/diff/40001/tools/gyp_flag_compare.py File tools/gyp_flag_compare.py (right): https://codereview.chromium.org/1988163002/diff/40001/tools/gyp_flag_compare.... tools/gyp_flag_compare.py:194: if len(sys.argv) < 4: Changing semantics a bit: Two different features: 1) Specify the gn and gyp out dirs on the cmd lines and expect everything to be generated already. Otherwise, this script would be too inflexible. 2) Switch targets for gn and gyp and allow to specify multiple targets for gyp. This accounts for gn's source set feature. In gn, e.g. the v8libplatform source set also indirectly includes the files of v8libbase, while in gyp, both are static libraries. In order for the tool to not drown you in diffs, we need to e.g. compare gn's libplatform with gyp's libplatform _and_ v8libbase targets. https://codereview.chromium.org/1988163002/diff/40001/tools/gyp_flag_compare.... tools/gyp_flag_compare.py:239: ['-I%s' % os.path.dirname(BASE_DIR)]) This silences a diff in the include dirs. In gn, the respective file is specified as a source explicitly: https://code.google.com/p/chromium/codesearch#chromium/src/v8/BUILD.gn&q=BUIL... In gyp, there is this extra include dir (which I don't wanna add in gn): https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/v8.gyp&l=390 The extra dir shows up in the diffs and puts the files from v8base and from e.g. v8libbase and v8platform into different groups. With this, the groups are merged and way easier to read.
lgtm https://codereview.chromium.org/1988163002/diff/40001/tools/gyp_flag_compare.py File tools/gyp_flag_compare.py (right): https://codereview.chromium.org/1988163002/diff/40001/tools/gyp_flag_compare.... tools/gyp_flag_compare.py:99: if rsp_file: Shouldn't this be above any of the other processing? (E.g., to determine whether we have any cc files, we need to read the .rsp file first?) https://codereview.chromium.org/1988163002/diff/40001/tools/gyp_flag_compare.... tools/gyp_flag_compare.py:162: global g_total_differences I'm not so fond of the global variable. What you could do instead is: - return a tuple, (output, local_differences) - in main(), when looping over the files, - have an empty list, differences = [] - append the tupels, differences.append(CompareLists(...)) - in main(), when you want to look at either the reports, or the sums: - difference_reports = ''.join(map(lambda (a,b): a), differences) - sum_of_differences = sum(map(lambda (a,b): b, differences) https://codereview.chromium.org/1988163002/diff/40001/tools/gyp_flag_compare.... tools/gyp_flag_compare.py:194: if len(sys.argv) < 4: On 2016/05/19 07:02:50, Michael Achenbach (slow) wrote: > Changing semantics a bit: Two different features: > 1) Specify the gn and gyp out dirs on the cmd lines and expect everything to be > generated already. Otherwise, this script would be too inflexible. > 2) Switch targets for gn and gyp and allow to specify multiple targets for gyp. > This accounts for gn's source set feature. In gn, e.g. the v8libplatform source > set also indirectly includes the files of v8libbase, while in gyp, both are > static libraries. In order for the tool to not drown you in diffs, we need to > e.g. compare gn's libplatform with gyp's libplatform _and_ v8libbase targets. Acknowledged. https://codereview.chromium.org/1988163002/diff/40001/tools/gyp_flag_compare.... tools/gyp_flag_compare.py:239: ['-I%s' % os.path.dirname(BASE_DIR)]) On 2016/05/19 07:02:50, Michael Achenbach (slow) wrote: > This silences a diff in the include dirs. > > In gn, the respective file is specified as a source explicitly: > https://code.google.com/p/chromium/codesearch#chromium/src/v8/BUILD.gn&q=BUIL... > > In gyp, there is this extra include dir (which I don't wanna add in gn): > https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/v8.gyp&l=390 > > The extra dir shows up in the diffs and puts the files from v8base and from e.g. > v8libbase and v8platform into different groups. With this, the groups are merged > and way easier to read. Acknowledged.
Thank! Hope you didn't review the whole thing in detail. The meaning of the two patchsets was to only look at the diff from 1-2. I should have stated that clearer, sorry. https://codereview.chromium.org/1988163002/diff/40001/tools/gyp_flag_compare.py File tools/gyp_flag_compare.py (right): https://codereview.chromium.org/1988163002/diff/40001/tools/gyp_flag_compare.... tools/gyp_flag_compare.py:99: if rsp_file: On 2016/05/19 08:06:06, vogelheim wrote: > Shouldn't this be above any of the other processing? (E.g., to determine whether > we have any cc files, we need to read the .rsp file first?) Maybe, didn't take windows into account at all yet. https://codereview.chromium.org/1988163002/diff/40001/tools/gyp_flag_compare.... tools/gyp_flag_compare.py:162: global g_total_differences On 2016/05/19 08:06:06, vogelheim wrote: > I'm not so fond of the global variable. What you could do instead is: > > - return a tuple, (output, local_differences) > - in main(), when looping over the files, > - have an empty list, differences = [] > - append the tupels, differences.append(CompareLists(...)) > - in main(), when you want to look at either the reports, or the sums: > - difference_reports = ''.join(map(lambda (a,b): a), differences) > - sum_of_differences = sum(map(lambda (a,b): b, differences) There are possibly some more improvements to be considered, but I don't want to change the original script's gory details. I didn't even look at them (didn't really want you to look at them either). If the original script changes, I'd like to have a rather easy life merging in the differences. If we want to change this, we should do it in the upstream version.
The CQ bit was checked by machenbach@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988163002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988163002/40001
Message was sent while issue was closed.
Description was changed from ========== [gn] Port gyp/gn comparison script BUG=chromium:474921 LOG=n NOTRY=true ========== to ========== [gn] Port gyp/gn comparison script BUG=chromium:474921 LOG=n NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [gn] Port gyp/gn comparison script BUG=chromium:474921 LOG=n NOTRY=true ========== to ========== [gn] Port gyp/gn comparison script BUG=chromium:474921 LOG=n NOTRY=true Committed: https://crrev.com/5e1c87dd9553a0ed3709caf706be300ab8c9b21a Cr-Commit-Position: refs/heads/master@{#36356} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5e1c87dd9553a0ed3709caf706be300ab8c9b21a Cr-Commit-Position: refs/heads/master@{#36356} |
