Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(340)

Issue 1135743005: Modify the gn version of 'mb analyze' to handle GN group targets as well. (Closed)

Created:
5 years, 7 months ago by Dirk Pranke
Modified:
5 years, 7 months ago
Reviewers:
brettw, scottmg
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@mb_response_file
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Modify the gn version of 'mb analyze' to handle GN group targets as well. Previously, if a group target like 'mandoline:all' was specified as an additional_compile_target that we wanted to build, the gn implementation of 'analyze' would not handle it correctly (it could only handle dependencies on files that existed in the build directory, like executables). This patch modifies the MB implementation to also handle label-like targets (the ninja mandoline:all target is the equivalent of the GN //mandonline:all target); we do this by calling 'gn refs' twice, once looking for a list of output paths to match against the compile targets and once looking for a list of phony labels to match against the compile targets. We also will match against just the target_name, so 'chrome_shell_apk' will also match "//chrome/android:chrome_shell_apk'. This may result in too many targets being rebuilt, but we can adjust that if need be. This is somewhat inefficient, but the alternatives would be to either 1) force the user to specify the stamp files for a group, which would be ugly, or 2) force the user to specify all the compile targets in terms of GN labels, which would be different from how the recipes work w/ GYP and possibly require us to map binary names to targets outside of GN, which would be a maintenance headache. R=scottmg@chromium.org,brettw@chromium.org BUG=487035 CQ_EXTRA_TRYBOTS=tryserver.chromium.mac:mac_chromium_gn_rel;tryserver.chromium.win:win8_chromium_gn_rel Committed: https://crrev.com/067d014b8f1918db09939aa765bdfa7576ab3d50 Cr-Commit-Position: refs/heads/master@{#329964}

Patch Set 1 #

Patch Set 2 : adjust so that we can match 'chrome_shell_apk' against '//chrome/android:chrome_shell_apk' #

Patch Set 3 : add the input target, not the build target, to the list of matches #

Patch Set 4 : add the input target, not the build target, to the list of matches #

Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -3 lines) Patch
M tools/mb/mb.py View 1 2 3 2 chunks +17 lines, -3 lines 0 comments Download

Messages

Total messages: 10 (4 generated)
Dirk Pranke
This patch depends on https://codereview.chromium.org/1131623006/ .
5 years, 7 months ago (2015-05-13 23:37:21 UTC) #1
scottmg
lgtm
5 years, 7 months ago (2015-05-13 23:59:38 UTC) #2
brettw
whatevs lgtm
5 years, 7 months ago (2015-05-14 16:38:26 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135743005/60001
5 years, 7 months ago (2015-05-14 21:39:57 UTC) #8
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years, 7 months ago (2015-05-14 22:52:54 UTC) #9
commit-bot: I haz the power
5 years, 7 months ago (2015-05-14 22:54:44 UTC) #10
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/067d014b8f1918db09939aa765bdfa7576ab3d50
Cr-Commit-Position: refs/heads/master@{#329964}

Powered by Google App Engine
This is Rietveld 408576698