Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2622583005:
Split renderer.lib on Windows to avoid files larger than 2GB. (Closed)
|
Created:
3 years, 11 months ago by Daniel Bratell Modified:
3 years, 11 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, mlamouri+watch-content_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSplit renderer.lib on Windows to avoid files larger than 2GB.
In certain configurations renderer.lib can exceed 2GB in size which is
too much for the 32 bit build tools. Splitting it avoids that problem.
BUG=679644
Review-Url: https://codereview.chromium.org/2622583005
Cr-Commit-Position: refs/heads/master@{#444143}
Committed: https://chromium.googlesource.com/chromium/src/+/99eedf26ca7066208384f7c46ca8dfd23242c2d4
Patch Set 1 #
Total comments: 4
Patch Set 2 : Review recommendations. #Patch Set 3 : Always split_static_library (unless component build). #Patch Set 4 : Rebased to a newer master #Messages
Total messages: 29 (19 generated)
The CQ bit was checked by bratell@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bratell@opera.com changed reviewers: + jochen@chromium.org
jochen, can you please take a look at this change from static_library to split_static_library?
jochen@chromium.org changed reviewers: + scottmg@chromium.org
deferring to scottmg lgtm if he approves as well
https://codereview.chromium.org/2622583005/diff/1/content/renderer/BUILD.gn File content/renderer/BUILD.gn (right): https://codereview.chromium.org/2622583005/diff/1/content/renderer/BUILD.gn#n... content/renderer/BUILD.gn:21: link_target_type = "static_library" Can you make this like browser https://cs.chromium.org/chromium/src/chrome/browser/BUILD.gn?rcl=0&l=59 so the type is always split_static_library?
https://codereview.chromium.org/2622583005/diff/1/content/renderer/BUILD.gn File content/renderer/BUILD.gn (right): https://codereview.chromium.org/2622583005/diff/1/content/renderer/BUILD.gn#n... content/renderer/BUILD.gn:393: if (is_win && !is_component_build) { If (per the bug) LTCG is mostly what makes this happen, then maybe use if (is_win && is_official_build) { to also match the browser.lib one.
The CQ bit was checked by bratell@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bratell@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
scottmg, consistency sounds like a good thing. Made it split always and started a test run. https://codereview.chromium.org/2622583005/diff/1/content/renderer/BUILD.gn File content/renderer/BUILD.gn (right): https://codereview.chromium.org/2622583005/diff/1/content/renderer/BUILD.gn#n... content/renderer/BUILD.gn:21: link_target_type = "static_library" On 2017/01/10 16:09:21, scottmg wrote: > Can you make this like browser > https://cs.chromium.org/chromium/src/chrome/browser/BUILD.gn?rcl=0&l=59 so the > type is always split_static_library? Done. https://codereview.chromium.org/2622583005/diff/1/content/renderer/BUILD.gn#n... content/renderer/BUILD.gn:393: if (is_win && !is_component_build) { On 2017/01/10 17:51:40, scottmg wrote: > If (per the bug) LTCG is mostly what makes this happen, then maybe use > > if (is_win && is_official_build) { > > to also match the browser.lib one. Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bratell@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2622583005/#ps40001 (title: "Always split_static_library (unless component build).")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for content/renderer/BUILD.gn:
While running git apply --index -p1;
error: patch failed: content/renderer/BUILD.gn:384
error: content/renderer/BUILD.gn: patch does not apply
Patch: content/renderer/BUILD.gn
Index: content/renderer/BUILD.gn
diff --git a/content/renderer/BUILD.gn b/content/renderer/BUILD.gn
index
3a28c2dd4b3ddce5c7b751ee35c409b395409127..f54730340bc0785cd1cdd4fab305a4b62db83c14
100644
--- a/content/renderer/BUILD.gn
+++ b/content/renderer/BUILD.gn
@@ -4,6 +4,7 @@
import("//build/config/features.gni")
import("//build/config/ui.gni")
+import("//build/split_static_library.gni")
import("//media/media_options.gni")
import("//ppapi/features/features.gni")
import("//printing/features/features.gni")
@@ -13,8 +14,9 @@ import("//tools/ipc_fuzzer/ipc_fuzzer.gni")
if (is_component_build) {
link_target_type = "source_set"
} else {
- link_target_type = "static_library"
+ link_target_type = "split_static_library"
}
+
target(link_target_type, "renderer") {
# Only the public target should depend on this. All other targets (even
# internal content ones) should depend on the public one.
@@ -384,6 +386,15 @@ target(link_target_type, "renderer") {
"websharedworker_proxy.h",
]
+ if (!is_component_build) {
+ if (is_win && is_official_build) {
+ split_count = 2 # In certain configurations a full renderer.lib can
+ # be 2+ GB which breaks some Windows tools.
+ } else {
+ split_count = 1
+ }
+ }
+
configs += [
"//content:content_implementation",
"//build/config/compiler:no_size_t_to_int_warning",
The CQ bit was checked by bratell@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/2622583005/#ps60001 (title: "Rebased to a newer master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484683466780860,
"parent_rev": "1c7224148cd93ce351941c1c3751dc58ca3cc6b7", "commit_rev":
"99eedf26ca7066208384f7c46ca8dfd23242c2d4"}
Message was sent while issue was closed.
Description was changed from ========== Split renderer.lib on Windows to avoid files larger than 2GB. In certain configurations renderer.lib can exceed 2GB in size which is too much for the 32 bit build tools. Splitting it avoids that problem. BUG=679644 ========== to ========== Split renderer.lib on Windows to avoid files larger than 2GB. In certain configurations renderer.lib can exceed 2GB in size which is too much for the 32 bit build tools. Splitting it avoids that problem. BUG=679644 Review-Url: https://codereview.chromium.org/2622583005 Cr-Commit-Position: refs/heads/master@{#444143} Committed: https://chromium.googlesource.com/chromium/src/+/99eedf26ca7066208384f7c46ca8... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/99eedf26ca7066208384f7c46ca8... |
