Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1658903002:
Enable /analyze in gn builds (Closed)
|
Created:
4 years, 10 months ago by brucedawson Modified:
4 years, 10 months ago Reviewers:
brettw CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable /analyze in gn builds
/analyze is disabled in ffmpeg because of an internal compiler error in
VS 2015 Update 1:
https://connect.microsoft.com/VisualStudio/feedback/details/2299303
The disabling of /analyze for ffmpeg will happen in its repo and
looks like this:
# /analyze must be disabled for ffmpeg because of an internal compiler
# error in VS 2015 Update 1, fixed in Update 2.
# https://connect.microsoft.com/VisualStudio/feedback/details/2299303
configs -= [ "//build/config/win:win_analyze" ]
See https://chromium-review.googlesource.com/#/c/328342/ for the
associated ffmpeg change.
BUG=427616, 582616
Committed: https://crrev.com/968cd5464ed5b4ea47b85ddf87645296e519e39e
Cr-Commit-Position: refs/heads/master@{#376583}
Patch Set 1 #Patch Set 2 : No change #Patch Set 3 : Switch to using configs #Patch Set 4 : Move checking of is_win_analyze to config #Patch Set 5 : gn format, and manual tweaks #
Total comments: 10
Patch Set 6 : Move and rename is_win_analyze #Patch Set 7 : Rename win:win_analyze to win:vs_code_analysis #Patch Set 8 : Comment reformat #Messages
Total messages: 19 (8 generated)
Description was changed from ========== Enable /analyze in gn builds R=dpranke@chromium.org BUG=427616 ========== to ========== Enable /analyze in gn builds BUG=427616 ==========
brucedawson@chromium.org changed reviewers: + brettw@chromium.org - dpranke@chromium.org
Description was changed from ========== Enable /analyze in gn builds BUG=427616 ========== to ========== Enable /analyze in gn builds BUG=427616,582616 ==========
Description was changed from ========== Enable /analyze in gn builds BUG=427616,582616 ========== to ========== Enable /analyze in gn builds /analyze is disabled in ffmpeg because of an internal compiler error in VS 2015 Update 1: https://connect.microsoft.com/VisualStudio/feedback/details/2299303 The disabling of /analyze for ffmpeg will happen in its repo and looks like this: if (is_win_analyze) { # /analyze must be disabled for ffmpeg because of an internal compiler # error in VS 2015 Update 1, fixed in Update 2. # https://connect.microsoft.com/VisualStudio/feedback/details/2299303 configs -= [ "//build/config/win:win_analyze" ] } BUG=427616,582616 ==========
With this change and the parallel change to ffmpeg's .gn file I can build all of Chrome with (VS 2015's) /analyze. I suspect I've got some stylistic or organizational mistakes in this - PTAL.
https://codereview.chromium.org/1658903002/diff/80001/build/config/BUILDCONFI... File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/1658903002/diff/80001/build/config/BUILDCONFI... build/config/BUILDCONFIG.gn:469: "//build/config/win:win_analyze", The only time we need to add stuff in this file is when some targets need to opt-out of the config. Otherwise it should go in one of the standard ones. https://codereview.chromium.org/1658903002/diff/80001/build/config/win/BUILD.gn File build/config/win/BUILD.gn (right): https://codereview.chromium.org/1658903002/diff/80001/build/config/win/BUILD.... build/config/win/BUILD.gn:14: config("compiler") { Probably the cleanest way to hook up your analyze config is to just reference it from this one which is already applied to all targets. To make a sub-config, just do configs = [ ":analyze" ] in this one. https://codereview.chromium.org/1658903002/diff/80001/build/config/win/BUILD.... build/config/win/BUILD.gn:83: config("win_analyze") { I'd call this just "analyze" since you're already in the "win" directory. https://codereview.chromium.org/1658903002/diff/80001/build/config/win/visual... File build/config/win/visual_studio_version.gni (right): https://codereview.chromium.org/1658903002/diff/80001/build/config/win/visual... build/config/win/visual_studio_version.gni:30: is_win_analyze = false Does this need to be usable by anywhere other than in the one BUILD file? If not, then move this declaration to that build file.
Thanks for the feedback. Here are my replies. For context, this is the CL to disable /analyze for ffmpeg until a VS 2015 bug is fixed. https://chromium-review.googlesource.com/#/c/326442/1/BUILD.gn https://codereview.chromium.org/1658903002/diff/80001/build/config/BUILDCONFI... File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/1658903002/diff/80001/build/config/BUILDCONFI... build/config/BUILDCONFIG.gn:469: "//build/config/win:win_analyze", On 2016/02/03 18:48:00, brettw wrote: > The only time we need to add stuff in this file is when some targets need to > opt-out of the config. Otherwise it should go in one of the standard ones. Some targets do indeed need to opt-out of this config - typically due to compiler bugs. Currently ffmpeg needs to opt-out. https://codereview.chromium.org/1658903002/diff/80001/build/config/win/BUILD.gn File build/config/win/BUILD.gn (right): https://codereview.chromium.org/1658903002/diff/80001/build/config/win/BUILD.... build/config/win/BUILD.gn:14: config("compiler") { On 2016/02/03 18:48:00, brettw wrote: > Probably the cleanest way to hook up your analyze config is to just reference it > from this one which is already applied to all targets. To make a sub-config, > just do > > configs = [ ":analyze" ] > > in this one. I think I had it there initially but then I couldn't remove it. https://codereview.chromium.org/1658903002/diff/80001/build/config/win/BUILD.... build/config/win/BUILD.gn:83: config("win_analyze") { On 2016/02/03 18:48:00, brettw wrote: > I'd call this just "analyze" since you're already in the "win" directory. I just tried that and immediately hit the disadvantage that grepping for it becomes much harder - 'analyze' is very generic. But, could go either way since "is_win_analyze" is still easy to search for. https://codereview.chromium.org/1658903002/diff/80001/build/config/win/visual... File build/config/win/visual_studio_version.gni (right): https://codereview.chromium.org/1658903002/diff/80001/build/config/win/visual... build/config/win/visual_studio_version.gni:30: is_win_analyze = false On 2016/02/03 18:48:00, brettw wrote: > Does this need to be usable by anywhere other than in the one BUILD file? If > not, then move this declaration to that build file. It's need by ffmpeg's build file.
Ping? Any thoughts on this?
Description was changed from ========== Enable /analyze in gn builds /analyze is disabled in ffmpeg because of an internal compiler error in VS 2015 Update 1: https://connect.microsoft.com/VisualStudio/feedback/details/2299303 The disabling of /analyze for ffmpeg will happen in its repo and looks like this: if (is_win_analyze) { # /analyze must be disabled for ffmpeg because of an internal compiler # error in VS 2015 Update 1, fixed in Update 2. # https://connect.microsoft.com/VisualStudio/feedback/details/2299303 configs -= [ "//build/config/win:win_analyze" ] } BUG=427616,582616 ========== to ========== Enable /analyze in gn builds /analyze is disabled in ffmpeg because of an internal compiler error in VS 2015 Update 1: https://connect.microsoft.com/VisualStudio/feedback/details/2299303 The disabling of /analyze for ffmpeg will happen in its repo and looks like this: # /analyze must be disabled for ffmpeg because of an internal compiler # error in VS 2015 Update 1, fixed in Update 2. # https://connect.microsoft.com/VisualStudio/feedback/details/2299303 configs -= [ "//build/config/win:win_analyze" ] See https://chromium-review.googlesource.com/#/c/328342/ for the associated ffmpeg change. BUG=427616,582616 ==========
On 2016/02/10 23:53:06, brucedawson wrote: > Ping? Any thoughts on this? I created a new ffmpeg change that unconditionally removes the analyze config. PTAL.
https://codereview.chromium.org/1658903002/diff/80001/build/config/win/visual... File build/config/win/visual_studio_version.gni (right): https://codereview.chromium.org/1658903002/diff/80001/build/config/win/visual... build/config/win/visual_studio_version.gni:30: is_win_analyze = false With our discussion in chat, I think you can make ffmpeg always remove this config, so this build flag can be moved to build/config/win/BUILD.gn and not shared in a .gni file. Also, can you call this maybe "use_vs_code_analysis"? The problem is that there are bot steps called "analyze" which seem possibly confusing with the presence of this name.
And now it is perfect! I renamed the config as well as the switch to make them consistent with each other. They are inconsistent with the gyp files, but oh well. https://codereview.chromium.org/1658903002/diff/80001/build/config/win/visual... File build/config/win/visual_studio_version.gni (right): https://codereview.chromium.org/1658903002/diff/80001/build/config/win/visual... build/config/win/visual_studio_version.gni:30: is_win_analyze = false On 2016/02/19 22:03:05, brettw wrote: > With our discussion in chat, I think you can make ffmpeg always remove this > config, so this build flag can be moved to build/config/win/BUILD.gn and not > shared in a .gni file. Ah... now I see the advantage of unconditionally removing this in ffmpeg. Okay, done. > Also, can you call this maybe "use_vs_code_analysis"? The problem is that there > are bot steps called "analyze" which seem possibly confusing with the presence > of this name. This is a tradeoff. On the one hand the compiler switch is called /analyze and that's how it's usually discussed, and the gyp files use win_analyze. On the other hand, confusion with the other analyze steps is real. I think going with vs_code_analysis makes sense, in which case the config should also be renamed to win:vs_code_analysis.
lgtm
The CQ bit was checked by brucedawson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1658903002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658903002/140001
Message was sent while issue was closed.
Description was changed from ========== Enable /analyze in gn builds /analyze is disabled in ffmpeg because of an internal compiler error in VS 2015 Update 1: https://connect.microsoft.com/VisualStudio/feedback/details/2299303 The disabling of /analyze for ffmpeg will happen in its repo and looks like this: # /analyze must be disabled for ffmpeg because of an internal compiler # error in VS 2015 Update 1, fixed in Update 2. # https://connect.microsoft.com/VisualStudio/feedback/details/2299303 configs -= [ "//build/config/win:win_analyze" ] See https://chromium-review.googlesource.com/#/c/328342/ for the associated ffmpeg change. BUG=427616,582616 ========== to ========== Enable /analyze in gn builds /analyze is disabled in ffmpeg because of an internal compiler error in VS 2015 Update 1: https://connect.microsoft.com/VisualStudio/feedback/details/2299303 The disabling of /analyze for ffmpeg will happen in its repo and looks like this: # /analyze must be disabled for ffmpeg because of an internal compiler # error in VS 2015 Update 1, fixed in Update 2. # https://connect.microsoft.com/VisualStudio/feedback/details/2299303 configs -= [ "//build/config/win:win_analyze" ] See https://chromium-review.googlesource.com/#/c/328342/ for the associated ffmpeg change. BUG=427616,582616 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Enable /analyze in gn builds /analyze is disabled in ffmpeg because of an internal compiler error in VS 2015 Update 1: https://connect.microsoft.com/VisualStudio/feedback/details/2299303 The disabling of /analyze for ffmpeg will happen in its repo and looks like this: # /analyze must be disabled for ffmpeg because of an internal compiler # error in VS 2015 Update 1, fixed in Update 2. # https://connect.microsoft.com/VisualStudio/feedback/details/2299303 configs -= [ "//build/config/win:win_analyze" ] See https://chromium-review.googlesource.com/#/c/328342/ for the associated ffmpeg change. BUG=427616,582616 ========== to ========== Enable /analyze in gn builds /analyze is disabled in ffmpeg because of an internal compiler error in VS 2015 Update 1: https://connect.microsoft.com/VisualStudio/feedback/details/2299303 The disabling of /analyze for ffmpeg will happen in its repo and looks like this: # /analyze must be disabled for ffmpeg because of an internal compiler # error in VS 2015 Update 1, fixed in Update 2. # https://connect.microsoft.com/VisualStudio/feedback/details/2299303 configs -= [ "//build/config/win:win_analyze" ] See https://chromium-review.googlesource.com/#/c/328342/ for the associated ffmpeg change. BUG=427616,582616 Committed: https://crrev.com/968cd5464ed5b4ea47b85ddf87645296e519e39e Cr-Commit-Position: refs/heads/master@{#376583} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/968cd5464ed5b4ea47b85ddf87645296e519e39e Cr-Commit-Position: refs/heads/master@{#376583} |
