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

Issue 1121453004: Fix GN visibility for a few targets that are compiled by default in a GN build. (Closed)

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

Description

Fix GN visibility for a few targets that are compiled by default in a GN build. When we build 'all' in GN, we can end up building targets that are defined in various build files even if they are not transitively referenced from 'gn_all' or 'gn_only'. This is undesirable because it makes it harder to tell when there might be discrepancies between the GN and GYP builds. Some of these targets may actually be hidden (using the visibility tag) and only intended to be depended on by other targets that don't work yet. By making this targets also visible to a top-level "//:gn_visibility" target we can ensure that we're tracking them properly without breaking the normal encapsulation rules. The two targets affected in this patch are - //build/config/sanitizers:options_sources - //ui/resources:repack_ui_test_mac_locale_pack R=brettw@chromium.org BUG=431177 CQ_EXTRA_TRYBOTS=tryserver.chromium.mac:mac_chromium_gn_rel,mac_chromium_gn_dbg Committed: https://crrev.com/b6128d057b09ade62d23528b6f919d5007f9defc Cr-Commit-Position: refs/heads/master@{#327922}

Patch Set 1 #

Patch Set 2 : fix build file #

Unified diffs Side-by-side diffs Delta from patch set Stats (+20 lines, -5 lines) Patch
M BUILD.gn View 1 4 chunks +12 lines, -3 lines 0 comments Download
M build/config/sanitizers/BUILD.gn View 1 chunk +4 lines, -1 line 0 comments Download
M ui/resources/BUILD.gn View 1 chunk +4 lines, -1 line 0 comments Download

Messages

Total messages: 10 (3 generated)
Dirk Pranke
Brett, are you okay with this approach? These two targets are trivially unimportant, but there ...
5 years, 7 months ago (2015-05-01 01:00:25 UTC) #1
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1121453004/20001
5 years, 7 months ago (2015-05-01 01:19:42 UTC) #3
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 7 months ago (2015-05-01 01:45:25 UTC) #5
brettw
lgtm
5 years, 7 months ago (2015-05-01 16:38:08 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1121453004/20001
5 years, 7 months ago (2015-05-01 16:39:25 UTC) #8
commit-bot: I haz the power
Committed patchset #2 (id:20001)
5 years, 7 months ago (2015-05-01 16:40:41 UTC) #9
commit-bot: I haz the power
5 years, 7 months ago (2015-05-01 16:41:54 UTC) #10
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/b6128d057b09ade62d23528b6f919d5007f9defc
Cr-Commit-Position: refs/heads/master@{#327922}

Powered by Google App Engine
This is Rietveld 408576698