Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1878273003:
Update blimp build bot args match build/args/blimp_engine.gn (Closed)
|
Created:
4 years, 8 months ago by maniscalco Modified:
4 years, 8 months ago Reviewers:
Dirk Pranke CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate blimp build bot args match build/args/blimp_engine.gn
Reorder args so it's easier to spot differences.
Remove gyp_defines because blimp is not designed to build with gyp.
BUG=602696
Committed: https://crrev.com/55bf089c901fb38204b642edc847b6259697ee9e
Cr-Commit-Position: refs/heads/master@{#386760}
Committed: https://crrev.com/aa26b499f5f41c9e62414496df5c4430f9b579cd
Cr-Commit-Position: refs/heads/master@{#387010}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add target_arch=unknown #Patch Set 3 : Now with more comma #Patch Set 4 : Add ozone_platform_headless=true which was left out of previous patch set #Patch Set 5 : Merge with master #
Messages
Total messages: 22 (11 generated)
Description was changed from ========== Update blimp build bot args match build/args/blimp_engine.gn Reorder args so it's easier to spot differences. Remove gyp_defines because blimp is not designed to build with gyp. BUG= ========== to ========== Update blimp build bot args match build/args/blimp_engine.gn Reorder args so it's easier to spot differences. Remove gyp_defines because blimp is not designed to build with gyp. BUG=602696 ==========
maniscalco@chromium.org changed reviewers: + dpranke@chromium.org
Dirk, would you please review this change? It's fallout from the recent ozone change. Two questions, 1. Is it safe to remove the gyp_defines entry as I've done in this CL? Blimp does not build with gyp at all so I'm assuming it's OK. Also, mb didn't complain when I ran it. 2. I believe you mentioned you were thinking about updating mb to support build/args/foo.gn files. Is that available yet? It would be great if we could define the args in one place (build/args/blimp_engine.gn) and then "include" that here instead of duplicating them. If not, I'd still like to land this patch as it will unblock our builds.
On 2016/04/12 17:55:43, maniscalco wrote: > Dirk, would you please review this change? > > It's fallout from the recent ozone change. > > Two questions, > > 1. Is it safe to remove the gyp_defines entry as I've done in this CL? Blimp > does not build with gyp at all so I'm assuming it's OK. Also, mb didn't > complain when I ran it. Yes, it's okay and allowed, but see the nit below for what I'd do instead. > > 2. I believe you mentioned you were thinking about updating mb to support > build/args/foo.gn files. Is that available yet? It would be great if we could > define the args in one place (build/args/blimp_engine.gn) and then "include" > that here instead of duplicating them. If not, I'd still like to land this > patch as it will unblock our builds. Yup, that'll be the "right" way to solve this issue, and I've posted a CL it: https://codereview.chromium.org/1881823002/ but it hasn't landed yet (hopefully will later today). lgtm to land this in the meantime. https://codereview.chromium.org/1878273003/diff/1/tools/mb/mb_config.pyl File tools/mb/mb_config.pyl (right): https://codereview.chromium.org/1878273003/diff/1/tools/mb/mb_config.pyl#newc... tools/mb/mb_config.pyl:1575: 'use_pulseaudio=false use_cups=false use_glib=false'), It's probably a bit safer to add: 'gyp_defines': 'target_arch=unknown' which will cause GYP to error out.
Oh cool, good timing. Thanks for the speedy review! https://codereview.chromium.org/1878273003/diff/1/tools/mb/mb_config.pyl File tools/mb/mb_config.pyl (right): https://codereview.chromium.org/1878273003/diff/1/tools/mb/mb_config.pyl#newc... tools/mb/mb_config.pyl:1575: 'use_pulseaudio=false use_cups=false use_glib=false'), On 2016/04/12 18:01:13, Dirk Pranke wrote: > It's probably a bit safer to add: > > 'gyp_defines': 'target_arch=unknown' > > which will cause GYP to error out. Good call. Done.
The CQ bit was checked by maniscalco@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/1878273003/#ps60001 (title: "Add ozone_platform_headless=true which was left out of previous patch set")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1878273003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1878273003/60001
Message was sent while issue was closed.
Description was changed from ========== Update blimp build bot args match build/args/blimp_engine.gn Reorder args so it's easier to spot differences. Remove gyp_defines because blimp is not designed to build with gyp. BUG=602696 ========== to ========== Update blimp build bot args match build/args/blimp_engine.gn Reorder args so it's easier to spot differences. Remove gyp_defines because blimp is not designed to build with gyp. BUG=602696 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Update blimp build bot args match build/args/blimp_engine.gn Reorder args so it's easier to spot differences. Remove gyp_defines because blimp is not designed to build with gyp. BUG=602696 ========== to ========== Update blimp build bot args match build/args/blimp_engine.gn Reorder args so it's easier to spot differences. Remove gyp_defines because blimp is not designed to build with gyp. BUG=602696 Committed: https://crrev.com/55bf089c901fb38204b642edc847b6259697ee9e Cr-Commit-Position: refs/heads/master@{#386760} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/55bf089c901fb38204b642edc847b6259697ee9e Cr-Commit-Position: refs/heads/master@{#386760}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1881343002/ by nednguyen@google.com. The reason for reverting is: Speculative revert: this breaks page_cycler.intl_hi_ru and page_cycler.intl_es_fr_pt-BR benchmarks.
Message was sent while issue was closed.
Description was changed from ========== Update blimp build bot args match build/args/blimp_engine.gn Reorder args so it's easier to spot differences. Remove gyp_defines because blimp is not designed to build with gyp. BUG=602696 Committed: https://crrev.com/55bf089c901fb38204b642edc847b6259697ee9e Cr-Commit-Position: refs/heads/master@{#386760} ========== to ========== Update blimp build bot args match build/args/blimp_engine.gn Reorder args so it's easier to spot differences. Remove gyp_defines because blimp is not designed to build with gyp. BUG=602696 Committed: https://crrev.com/55bf089c901fb38204b642edc847b6259697ee9e Cr-Commit-Position: refs/heads/master@{#386760} ==========
On 2016/04/13 04:06:03, nednguyen wrote: > A revert of this CL (patchset #4 id:60001) has been created in > https://codereview.chromium.org/1881343002/ by mailto:nednguyen@google.com. > > The reason for reverting is: Speculative revert: this breaks > page_cycler.intl_hi_ru and page_cycler.intl_es_fr_pt-BR benchmarks. For reference, the bug for the page_cycler benchmark failures is https://bugs.chromium.org/p/chromium/issues/detail?id=602801 I'm relanding this patch as I'm confident it was the the cause of the benchmark failures.
The CQ bit was checked by maniscalco@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/1878273003/#ps80001 (title: "Merge with master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1878273003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1878273003/80001
Message was sent while issue was closed.
Description was changed from ========== Update blimp build bot args match build/args/blimp_engine.gn Reorder args so it's easier to spot differences. Remove gyp_defines because blimp is not designed to build with gyp. BUG=602696 Committed: https://crrev.com/55bf089c901fb38204b642edc847b6259697ee9e Cr-Commit-Position: refs/heads/master@{#386760} ========== to ========== Update blimp build bot args match build/args/blimp_engine.gn Reorder args so it's easier to spot differences. Remove gyp_defines because blimp is not designed to build with gyp. BUG=602696 Committed: https://crrev.com/55bf089c901fb38204b642edc847b6259697ee9e Cr-Commit-Position: refs/heads/master@{#386760} ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Update blimp build bot args match build/args/blimp_engine.gn Reorder args so it's easier to spot differences. Remove gyp_defines because blimp is not designed to build with gyp. BUG=602696 Committed: https://crrev.com/55bf089c901fb38204b642edc847b6259697ee9e Cr-Commit-Position: refs/heads/master@{#386760} ========== to ========== Update blimp build bot args match build/args/blimp_engine.gn Reorder args so it's easier to spot differences. Remove gyp_defines because blimp is not designed to build with gyp. BUG=602696 Committed: https://crrev.com/55bf089c901fb38204b642edc847b6259697ee9e Cr-Commit-Position: refs/heads/master@{#386760} Committed: https://crrev.com/aa26b499f5f41c9e62414496df5c4430f9b579cd Cr-Commit-Position: refs/heads/master@{#387010} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/aa26b499f5f41c9e62414496df5c4430f9b579cd Cr-Commit-Position: refs/heads/master@{#387010} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
