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

Issue 1260793006: Revert of Add precompiled headers to GN build for large targets. (Closed)

Created:
5 years, 4 months ago by Dirk Pranke
Modified:
5 years, 4 months ago
Reviewers:
brettw
CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, zea+watch_chromium.org, tzik, binji+watch_chromium.org, bradnelson+warch_chromium.org, browser-components-watch_chromium.org, teravest+watch_chromium.org, tim+watch_chromium.org, extensions-reviews_chromium.org, cbentzel+watch_chromium.org, grt+watch_chromium.org, chromoting-reviews_chromium.org, jam, pvalenzuela+watch_chromium.org, darin-cc_chromium.org, rouslan+autofillwatch_chromium.org, devtools-reviews_chromium.org, chromium-apps-reviews_chromium.org, erikwright+watch_chromium.org, ihf+watch_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-notifications_chromium.org, rickyz+watch_chromium.org, feature-media-reviews_chromium.org, kalyank, piman+watch_chromium.org, mkwst+moarreviews-shell_chromium.org, peter+watch_chromium.org, jochen+watch_chromium.org, maniscalco+watch_chromium.org, wfh+watch_chromium.org, maxbogue+watch_chromium.org, yurys, plaree+watch_chromium.org, tfarina, estade+watch_chromium.org, cc-bugs_chromium.org, pfeldman
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Revert of Add precompiled headers to GN build for large targets. (patchset #5 id:80001 of https://codereview.chromium.org/1250273002/) Reason for revert: Speculatively reverting this, in thinking that maybe this broke incremental builds on the Win GN bots, e.g.: http://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN/builds/4709 Original issue's description: > Add precompiled headers to GN build for large targets. > > Turns on precompiled header support in the GN build on Windows, and adds the precompiled header config to most large-ish targets in the build. > > Removes Windows files from the precompiled header. This does not seem to affect the build speed much because most Chrome files don't depend on Windows any more. And windows.h injects typedefs and defines that conflict with some third party libraries and prevent using precompiled headers for those targets or any target that includes them. > > I counted ~50 files or bigger as large. The 50 file threshold is based on some previous approximate measurements (since the precompile step is an extra per-target compile, it can actually make small targets compile slower). > > For borderline cases, I added the precompiled header flag if I thought it was likely to have more files added, and didn't add it if I thought the target was likely to be static. > > CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel > > Committed: https://crrev.com/8f3218985dde74063ccc362da47803be163f3165 > Cr-Commit-Position: refs/heads/master@{#340535} TBR=brettw@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+108 lines, -188 lines) Patch
M ash/BUILD.gn View 3 chunks +1 line, -4 lines 0 comments Download
M base/BUILD.gn View 1 chunk +1 line, -4 lines 0 comments Download
M base/test/BUILD.gn View 1 chunk +0 lines, -2 lines 0 comments Download
M build/config/BUILD.gn View 1 chunk +3 lines, -1 line 0 comments Download
M build/config/compiler/BUILD.gn View 1 chunk +0 lines, -6 lines 0 comments Download
M build/json_schema_api.gni View 2 chunks +0 lines, -6 lines 0 comments Download
M build/precompile.h View 3 chunks +69 lines, -17 lines 0 comments Download
M build/toolchain/win/BUILD.gn View 2 chunks +6 lines, -2 lines 0 comments Download
M cc/BUILD.gn View 3 chunks +0 lines, -4 lines 0 comments Download
M chrome/browser/BUILD.gn View 2 chunks +0 lines, -3 lines 0 comments Download
M chrome/browser/devtools/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/extensions/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/ui/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/common/BUILD.gn View 1 chunk +1 line, -4 lines 0 comments Download
M chrome/common/extensions/api/BUILD.gn View 2 chunks +0 lines, -2 lines 0 comments Download
M chrome/installer/util/BUILD.gn View 1 chunk +2 lines, -6 lines 0 comments Download
M chrome/test/BUILD.gn View 4 chunks +0 lines, -6 lines 0 comments Download
M chrome/utility/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M components/autofill/core/browser/BUILD.gn View 1 chunk +0 lines, -2 lines 0 comments Download
M components/policy/core/common/BUILD.gn View 1 chunk +0 lines, -2 lines 0 comments Download
M content/browser/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M content/child/BUILD.gn View 1 chunk +0 lines, -2 lines 0 comments Download
M content/common/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M content/public/browser/BUILD.gn View 1 chunk +1 line, -4 lines 0 comments Download
M content/public/common/BUILD.gn View 1 chunk +1 line, -4 lines 0 comments Download
M content/shell/BUILD.gn View 1 chunk +2 lines, -6 lines 0 comments Download
M content/test/BUILD.gn View 3 chunks +1 line, -8 lines 0 comments Download
M extensions/BUILD.gn View 1 chunk +0 lines, -2 lines 0 comments Download
M extensions/browser/BUILD.gn View 1 chunk +2 lines, -6 lines 0 comments Download
M extensions/common/BUILD.gn View 1 chunk +2 lines, -6 lines 0 comments Download
M extensions/renderer/BUILD.gn View 1 chunk +2 lines, -6 lines 0 comments Download
M gpu/BUILD.gn View 1 chunk +1 line, -6 lines 0 comments Download
M gpu/command_buffer/service/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M media/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M media/base/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M net/BUILD.gn View 3 chunks +6 lines, -18 lines 0 comments Download
M ppapi/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M ppapi/cpp/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M ppapi/proxy/BUILD.gn View 1 chunk +1 line, -4 lines 0 comments Download
M ppapi/shared_impl/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M ppapi/thunk/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M remoting/protocol/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M sandbox/win/BUILD.gn View 1 chunk +0 lines, -2 lines 0 comments Download
M skia/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M storage/browser/BUILD.gn View 1 chunk +0 lines, -2 lines 0 comments Download
M sync/BUILD.gn View 2 chunks +0 lines, -4 lines 0 comments Download
M ui/gfx/BUILD.gn View 1 chunk +2 lines, -6 lines 0 comments Download
M ui/gl/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M ui/message_center/BUILD.gn View 1 chunk +2 lines, -6 lines 0 comments Download
M ui/views/BUILD.gn View 2 chunks +2 lines, -8 lines 0 comments Download
M ui/wm/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 5 (2 generated)
Dirk Pranke
Created Revert of Add precompiled headers to GN build for large targets.
5 years, 4 months ago (2015-07-28 00:48:04 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1260793006/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1260793006/1
5 years, 4 months ago (2015-07-28 00:48:15 UTC) #2
commit-bot: I haz the power
5 years, 4 months ago (2015-07-28 00:49:31 UTC) #4
Failed to apply patch for content/test/BUILD.gn:
While running git apply --index -3 -p1;
  error: patch failed: content/test/BUILD.gn:431
  Falling back to three-way merge...
  Applied patch to 'content/test/BUILD.gn' with conflicts.
  U content/test/BUILD.gn

Patch:       content/test/BUILD.gn
Index: content/test/BUILD.gn
diff --git a/content/test/BUILD.gn b/content/test/BUILD.gn
index
23c600cc56b9d1b066d4e1c55bdf940211b591e4..0dbffbdb0022a76a505f145e135c6838fb53a41f
100644
--- a/content/test/BUILD.gn
+++ b/content/test/BUILD.gn
@@ -22,7 +22,6 @@
 # GYP version //content/content_tests.gypi:test_support_content
 source_set("test_support") {
   testonly = true
-  configs += [ "//build/config:precompiled_headers" ]
   public_deps = [
     "//content/public/app:both",
     "//content/public/browser",
@@ -269,10 +268,7 @@
 
     defines = [ "HAS_OUT_OF_PROC_TEST_RUNNER" ]
 
-    configs += [
-      "//build/config:precompiled_headers",
-      "//build/config/compiler:no_size_t_to_int_warning",
-    ]
+    configs += [ "//build/config/compiler:no_size_t_to_int_warning" ]
 
     deps = [
       ":browsertest_support",
@@ -431,9 +427,6 @@
   sources = rebase_path(content_tests_gypi_values.content_unittests_sources,
                         ".",
                         "//content")
-
-  configs += [ "//build/config:precompiled_headers" ]
-
   deps = [
     ":test_support",
     "//base/allocator",

Powered by Google App Engine
This is Rietveld 408576698