Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2917483002:
Fix Ozone build (Closed)
DescriptionFix Ozone build.
The #if guards on the function which records the linux distro are different to the guards on the function call. This results in the function being unused in the ozone build, which is a warning (turned into an error) causing the build to fail.
The fix is simply to unify the conditions of the #if guards.
BUG=724244
R=thomasanderson@chromium.org,rkaplow@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_ozone_compile_only_ng
Review-Url: https://codereview.chromium.org/2917483002
Cr-Commit-Position: refs/heads/master@{#476332}
Committed: https://chromium.googlesource.com/chromium/src/+/238aaaaf4fafed90ffe478d24fe9a17e61043755
Patch Set 1 #
Total comments: 2
Patch Set 2 : Split guards #Patch Set 3 : Review comment changes #
Total comments: 1
Messages
Total messages: 23 (14 generated)
The CQ bit was checked by timbrown@chromium.org 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...
Description was changed from ========== Fix Ozone build. BUG=724244 R=thomasanderson@chromium.org,rkaplow@chromium.org ========== to ========== Fix Ozone build. BUG=724244 R=thomasanderson@chromium.org,rkaplow@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_ozone_compile_only_rel_ng ==========
lgtm https://codereview.chromium.org/2917483002/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2917483002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:471: #if defined(USE_X11) GetLinuxWindowManager above is guarded with "defined(USE_X11) && !defined(OS_CHROMEOS)", but here it effectively has "defined(OS_LINUX) && !defined(OS_CHROMEOS) && defined(USE_X11)", but this is probably OK because X11 is only used on Linux. Personally I'd recommend splitting it out because it's easier to understand.
https://codereview.chromium.org/2917483002/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2917483002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:471: #if defined(USE_X11) On 2017/05/30 22:57:07, Tom Anderson wrote: > GetLinuxWindowManager above is guarded with "defined(USE_X11) && > !defined(OS_CHROMEOS)", but here it effectively has "defined(OS_LINUX) && > !defined(OS_CHROMEOS) && defined(USE_X11)", but this is probably OK because X11 > is only used on Linux. Personally I'd recommend splitting it out because it's > easier to understand. Sure. Done.
The CQ bit was checked by timbrown@chromium.org 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: Try jobs failed on following builders: linux_chromium_ozone_compile_only_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
Description was changed from ========== Fix Ozone build. BUG=724244 R=thomasanderson@chromium.org,rkaplow@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_ozone_compile_only_rel_ng ========== to ========== Fix Ozone build. BUG=724244 R=thomasanderson@chromium.org,rkaplow@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_ozone_compile_only_ng ==========
On 2017/05/30 23:03:58, timbrown wrote: > https://codereview.chromium.org/2917483002/diff/1/chrome/browser/metrics/chro... > File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): > > https://codereview.chromium.org/2917483002/diff/1/chrome/browser/metrics/chro... > chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:471: #if > defined(USE_X11) > On 2017/05/30 22:57:07, Tom Anderson wrote: > > GetLinuxWindowManager above is guarded with "defined(USE_X11) && > > !defined(OS_CHROMEOS)", but here it effectively has "defined(OS_LINUX) && > > !defined(OS_CHROMEOS) && defined(USE_X11)", but this is probably OK because > X11 > > is only used on Linux. Personally I'd recommend splitting it out because it's > > easier to understand. > > Sure. Done. FWIW in my opinion it looks strange this way since now we have a Linux section not under the LINUX guard, even though the X11 implies it. Wonder if it would better to have the first guard be #if defined(LINUX) && if defined(USE_X11) && !defined(OS_CHROMEOS) even if it's a bit redundant, in case something else ever uses X11 (and it looks less wrong)
lgtm
On 2017/05/31 19:04:12, rkaplow wrote: > On 2017/05/30 23:03:58, timbrown wrote: > > > https://codereview.chromium.org/2917483002/diff/1/chrome/browser/metrics/chro... > > File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc > (right): > > > > > https://codereview.chromium.org/2917483002/diff/1/chrome/browser/metrics/chro... > > chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:471: #if > > defined(USE_X11) > > On 2017/05/30 22:57:07, Tom Anderson wrote: > > > GetLinuxWindowManager above is guarded with "defined(USE_X11) && > > > !defined(OS_CHROMEOS)", but here it effectively has "defined(OS_LINUX) && > > > !defined(OS_CHROMEOS) && defined(USE_X11)", but this is probably OK because > > X11 > > > is only used on Linux. Personally I'd recommend splitting it out because > it's > > > easier to understand. > > > > Sure. Done. > > FWIW in my opinion it looks strange this way since now we have a Linux section > not under the LINUX guard, even though the X11 implies it. Wonder if it would > better to have the first guard be > #if defined(LINUX) && if defined(USE_X11) && !defined(OS_CHROMEOS) > even if it's a bit redundant, in case something else ever uses X11 (and it looks > less wrong) Done.
The CQ bit was checked by timbrown@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thomasanderson@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2917483002/#ps40001 (title: "Review comment changes")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix Ozone build. BUG=724244 R=thomasanderson@chromium.org,rkaplow@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_ozone_compile_only_ng ========== to ========== Fix Ozone build. The #if guards on the function which records the linux distro are different to the guards on the function call. This results in the function being unused in the ozone build, which is a warning (turned into an error) causing the build to fail. The fix is simply to unify the conditions of the #if guards. BUG=724244 R=thomasanderson@chromium.org,rkaplow@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_ozone_compile_only_ng ==========
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496333313050040,
"parent_rev": "a7dc4e822a4cddc14b9929a6f7aa2a85e69252d8", "commit_rev":
"238aaaaf4fafed90ffe478d24fe9a17e61043755"}
Message was sent while issue was closed.
Description was changed from ========== Fix Ozone build. The #if guards on the function which records the linux distro are different to the guards on the function call. This results in the function being unused in the ozone build, which is a warning (turned into an error) causing the build to fail. The fix is simply to unify the conditions of the #if guards. BUG=724244 R=thomasanderson@chromium.org,rkaplow@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_ozone_compile_only_ng ========== to ========== Fix Ozone build. The #if guards on the function which records the linux distro are different to the guards on the function call. This results in the function being unused in the ozone build, which is a warning (turned into an error) causing the build to fail. The fix is simply to unify the conditions of the #if guards. BUG=724244 R=thomasanderson@chromium.org,rkaplow@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_ozone_compile_only_ng Review-Url: https://codereview.chromium.org/2917483002 Cr-Commit-Position: refs/heads/master@{#476332} Committed: https://chromium.googlesource.com/chromium/src/+/238aaaaf4fafed90ffe478d24fe9... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/238aaaaf4fafed90ffe478d24fe9...
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
nit: https://codereview.chromium.org/2917483002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2917483002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:286: #if defined(USE_X11) && !defined(OS_CHROMEOS) This should match line 467. |
