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

Unified Diff: chrome/installer/util/BUILD.gn

Issue 1534903002: Split GN installer_util into one that has no strings. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Created 5 years ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « chrome/chrome_watcher/BUILD.gn ('k') | win8/delegate_execute/BUILD.gn » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: chrome/installer/util/BUILD.gn
diff --git a/chrome/installer/util/BUILD.gn b/chrome/installer/util/BUILD.gn
index ca137997c46c8cb6caabe52c4b23e36d6fe80f1d..985afa4f81ce99afa21951f08a7638658a68f4fa 100644
--- a/chrome/installer/util/BUILD.gn
+++ b/chrome/installer/util/BUILD.gn
@@ -5,7 +5,37 @@
import("//build/config/chrome_build.gni")
import("//testing/test.gni")
-static_library("util") {
+# Generally you should depend on this target, see below for discussion.
+group("util") {
+ public_deps = [
+ ":with_no_strings_some_things_wont_work",
+ ]
+ if (is_win) {
+ public_deps += [ ":strings" ]
+ }
+}
+
+# chrome/installer/util has generated strings on Windows. These appear as
grt (UTC plus 2) 2015/12/18 19:02:15 I think this comment is incorrect. Chrome calls ma
brettw 2015/12/21 17:33:00 This comment is referring specifically to chrome.e
grt (UTC plus 2) 2015/12/21 19:45:06 Ah, now I see what you mean. :-(
brettw 2015/12/21 22:56:22 I made this distinction more clear in the comment
+# Windows resources and are fairly large (~200KB). They are generated by the
+# ":generate_strings" target and compiled by the ":strings" target.
+#
+# Some code, like the Windows chrome.exe code uses installer_util functions
+# without calling any functions that use the strings. And historitally in the
Dirk Pranke 2015/12/20 19:53:46 nit: s/historitally/historically
+# GYP build the strings resource had to be manually linked, so the strings
+# never ended up being in chrome.exe and everything was fine.
+#
+# However, this is obviously a fragile and confusing situation. If you depend
+# on this target rather than going through the ":util" version that links the
+# strings above, anything that uses localized strings won't work. There is
+# no definition of what works and doesn't work, and this may also change over
+# time. As an example at the time of this writing, chrome.exe calls
+# BrowserDistribution::GetRegistryPath. This function doesn't use any strings,
+# but lots of other functions in BrowserDistribution do use localized strings.
+#
+# Ideally, this should be cleaved in two parts: the main "installer util" and
+# some kind of mini installer util that just contains the functions needed by
+# chrome.exe and any other clients that don't need the strings.
+static_library("with_no_strings_some_things_wont_work") {
deps = [
"//base",
"//chrome:strings",
@@ -66,10 +96,12 @@ static_library("util") {
"user_experiment.h",
]
- public_deps = [
- ":strings",
- ]
deps += [
+ # Need to depend on the generated strings target since files here
+ # depend on the generated header, but only depend on the ":strings"
+ # target (which actually compiles and causes the generated code to be
+ # linked) from the ":util" target.
+ ":generate_strings",
"//base/third_party/dynamic_annotations",
"//components/metrics",
"//courgette:courgette_lib",
@@ -187,7 +219,10 @@ static_library("util") {
}
action("generate_strings") {
- visibility = [ ":strings" ]
+ visibility = [
+ ":strings",
+ ":with_no_strings_some_things_wont_work",
+ ]
script = "prebuild/create_string_rc.py"
if (is_chrome_branded) {
« no previous file with comments | « chrome/chrome_watcher/BUILD.gn ('k') | win8/delegate_execute/BUILD.gn » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698