Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2891533002:
[Cronet] Use is_official_build=true for Cronet builds. (Closed)
Description[Cronet] Use is_official_build=true for Cronet builds.
BUG=722868
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Review-Url: https://codereview.chromium.org/2891533002
Cr-Commit-Position: refs/heads/master@{#472578}
Committed: https://chromium.googlesource.com/chromium/src/+/0c837199faf1546a1d7354eab6a9de9f66ec5f2b
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use is_official_build only for relese builds. #
Messages
Total messages: 19 (10 generated)
Description was changed from ========== [Cronet] Use is_official_build=true for Cronet builds. BUG=722868 ========== to ========== [Cronet] Use is_official_build=true for Cronet builds. BUG=722868 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
The CQ bit was checked by mef@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...
mef@chromium.org changed reviewers: + jbudorick@chromium.org, mgersh@chromium.org
PTAL. How do I test the mb config changes?
https://codereview.chromium.org/2891533002/diff/1/components/cronet/tools/cr_... File components/cronet/tools/cr_cronet.py (right): https://codereview.chromium.org/2891533002/diff/1/components/cronet/tools/cr_... components/cronet/tools/cr_cronet.py:98: gn_args = 'is_official_build=true use_errorprone_java_compiler=true arm_use_neon=false ' Should this be only on release builds?
You can test this locally by running ./tools/mb/mb.py gen -m chromium.android -b <bot name> <output directory> ninja -C <output directory> [targets & other args] https://codereview.chromium.org/2891533002/diff/1/tools/mb/mb_config.pyl File tools/mb/mb_config.pyl (right): https://codereview.chromium.org/2891533002/diff/1/tools/mb/mb_config.pyl#newc... tools/mb/mb_config.pyl:738: 'android', 'cronet', 'official_optimize', 'release_bot', 'minimal_symbols', 'arm64', official_optimize and not official?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
mef@chromium.org changed reviewers: + gab@chromium.org
PTAL. https://codereview.chromium.org/2891533002/diff/1/components/cronet/tools/cr_... File components/cronet/tools/cr_cronet.py (right): https://codereview.chromium.org/2891533002/diff/1/components/cronet/tools/cr_... components/cronet/tools/cr_cronet.py:98: gn_args = 'is_official_build=true use_errorprone_java_compiler=true arm_use_neon=false ' On 2017/05/16 21:22:55, mgersh wrote: > Should this be only on release builds? Done. https://codereview.chromium.org/2891533002/diff/1/tools/mb/mb_config.pyl File tools/mb/mb_config.pyl (right): https://codereview.chromium.org/2891533002/diff/1/tools/mb/mb_config.pyl#newc... tools/mb/mb_config.pyl:738: 'android', 'cronet', 'official_optimize', 'release_bot', 'minimal_symbols', 'arm64', On 2017/05/16 22:00:42, jbudorick wrote: > official_optimize and not official? Hrm, good question. What does is_chrome_branded=true do? How would it affect base/ or /net?
lgtm
lgtm We'll also want to update https://codesearch.chromium.org/chromium/build/scripts/slave/recipe_modules/c... to address internal. On 2017/05/17 16:01:14, mef wrote: > PTAL. > > https://codereview.chromium.org/2891533002/diff/1/components/cronet/tools/cr_... > File components/cronet/tools/cr_cronet.py (right): > > https://codereview.chromium.org/2891533002/diff/1/components/cronet/tools/cr_... > components/cronet/tools/cr_cronet.py:98: gn_args = 'is_official_build=true > use_errorprone_java_compiler=true arm_use_neon=false ' > On 2017/05/16 21:22:55, mgersh wrote: > > Should this be only on release builds? > > Done. > > https://codereview.chromium.org/2891533002/diff/1/tools/mb/mb_config.pyl > File tools/mb/mb_config.pyl (right): > > https://codereview.chromium.org/2891533002/diff/1/tools/mb/mb_config.pyl#newc... > tools/mb/mb_config.pyl:738: 'android', 'cronet', 'official_optimize', > 'release_bot', 'minimal_symbols', 'arm64', > On 2017/05/16 22:00:42, jbudorick wrote: > > official_optimize and not official? > > Hrm, good question. What does is_chrome_branded=true do? > How would it affect base/ or /net? At a glance, I think the only ways it'd affect //base or //net would be w/ some information we exclude from official builds: https://codesearch.chromium.org/search/?q=is_chrome_branded+package:%5Echromi... (particularly exclude_unwind_tables) That said, this is probably safer as-is. We can follow up separately on is_chrome_branded if you'd like.
On 2017/05/17 17:02:27, jbudorick wrote: > lgtm > > We'll also want to update > https://codesearch.chromium.org/chromium/build/scripts/slave/recipe_modules/c... > to address internal. Yep, I've created a separate CL for that. > > On 2017/05/17 16:01:14, mef wrote: > > PTAL. > > > > > https://codereview.chromium.org/2891533002/diff/1/components/cronet/tools/cr_... > > File components/cronet/tools/cr_cronet.py (right): > > > > > https://codereview.chromium.org/2891533002/diff/1/components/cronet/tools/cr_... > > components/cronet/tools/cr_cronet.py:98: gn_args = 'is_official_build=true > > use_errorprone_java_compiler=true arm_use_neon=false ' > > On 2017/05/16 21:22:55, mgersh wrote: > > > Should this be only on release builds? > > > > Done. > > > > https://codereview.chromium.org/2891533002/diff/1/tools/mb/mb_config.pyl > > File tools/mb/mb_config.pyl (right): > > > > > https://codereview.chromium.org/2891533002/diff/1/tools/mb/mb_config.pyl#newc... > > tools/mb/mb_config.pyl:738: 'android', 'cronet', 'official_optimize', > > 'release_bot', 'minimal_symbols', 'arm64', > > On 2017/05/16 22:00:42, jbudorick wrote: > > > official_optimize and not official? > > > > Hrm, good question. What does is_chrome_branded=true do? > > How would it affect base/ or /net? > > At a glance, I think the only ways it'd affect //base or //net would be w/ some > information we exclude from official builds: > https://codesearch.chromium.org/search/?q=is_chrome_branded+package:%5Echromi... > (particularly exclude_unwind_tables) Ah, Cronet needs unwind tables as it is not using breakpad and crashpad is not ready yet. > > That said, this is probably safer as-is. We can follow up separately on > is_chrome_branded if you'd like. Yep.
The CQ bit was checked by mef@chromium.org
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": 20001, "attempt_start_ts": 1495041637732070,
"parent_rev": "a5167787952f3605a0bcf6d634fc781db3bbea57", "commit_rev":
"0c837199faf1546a1d7354eab6a9de9f66ec5f2b"}
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Use is_official_build=true for Cronet builds. BUG=722868 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== [Cronet] Use is_official_build=true for Cronet builds. BUG=722868 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2891533002 Cr-Commit-Position: refs/heads/master@{#472578} Committed: https://chromium.googlesource.com/chromium/src/+/0c837199faf1546a1d7354eab6a9... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/0c837199faf1546a1d7354eab6a9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
