Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1776933002:
Add the missing asset file cast_shell.pak back (Closed)
|
Created:
4 years, 9 months ago by AndyWu Modified:
4 years, 9 months ago CC:
chromium-reviews, gunsch+watch_chromium.org, lcwu+watch_chromium.org, halliwell+watch_chromium.org, Simeon Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd the missing asset file cast_shell.pak back
All files under assets directory will be removed when building cast_shell_icudata target. We have to make sure all the asset files must be collected here.
BUG=593277
Committed: https://crrev.com/c058232e6063829ab7db5dab45bdbdbb988b06d0
Cr-Commit-Position: refs/heads/master@{#380202}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #Patch Set 3 : #Messages
Total messages: 25 (12 generated)
Description was changed from ========== Avoid removing asset files when building cast_shell_icudata In case of cast_shell_pak built prior to cast_shell_icudata, the cast_shell.apk will be removed from assets directory when building cast_shell_icudata. We should not clear the assets directory as building cast_shell_icudata. BUG=593277 ========== to ========== Avoid removing asset files when building cast_shell_icudata In case of cast_shell_pak built prior to cast_shell_icudata, the cast_shell.apk will be removed from assets directory when building cast_shell_icudata. We should not clear the assets directory as building cast_shell_icudata. BUG=593277 ==========
tsunghung@chromium.org changed reviewers: + michaelbai@chromium.org
sanfin@chromium.org changed reviewers: + halliwell@chromium.org, sanfin@chromium.org
Good catch, Andy. lgtm
On 2016/03/09 17:43:23, Simeon wrote: > Good catch, Andy. lgtm lgtm
https://codereview.chromium.org/1776933002/diff/1/chromecast/chromecast.gyp File chromecast/chromecast.gyp (left): https://codereview.chromium.org/1776933002/diff/1/chromecast/chromecast.gyp#o... chromecast/chromecast.gyp:582: 'clear': 1, You should copy cast_shell.pak and any other assets to assets directory here, instead of removing clear parameter, this will avoid potential issue in the future.
Description was changed from ========== Avoid removing asset files when building cast_shell_icudata In case of cast_shell_pak built prior to cast_shell_icudata, the cast_shell.apk will be removed from assets directory when building cast_shell_icudata. We should not clear the assets directory as building cast_shell_icudata. BUG=593277 ========== to ========== Add the missing asset file cast_shell.pak back All files under assets directory will be removed when building cast_shell_icudata target. We have to make sure all the asset files must be collected here. BUG=593277 ==========
https://codereview.chromium.org/1776933002/diff/1/chromecast/chromecast.gyp File chromecast/chromecast.gyp (left): https://codereview.chromium.org/1776933002/diff/1/chromecast/chromecast.gyp#o... chromecast/chromecast.gyp:582: 'clear': 1, On 2016/03/09 17:56:09, michaelbai wrote: > You should copy cast_shell.pak and any other assets to assets directory here, > instead of removing clear parameter, this will avoid potential issue in the > future. Done. But the clear flag makes issue like this very difficult to figure out.
On 2016/03/09 19:39:02, AndyWu wrote: > https://codereview.chromium.org/1776933002/diff/1/chromecast/chromecast.gyp > File chromecast/chromecast.gyp (left): > > https://codereview.chromium.org/1776933002/diff/1/chromecast/chromecast.gyp#o... > chromecast/chromecast.gyp:582: 'clear': 1, > On 2016/03/09 17:56:09, michaelbai wrote: > > You should copy cast_shell.pak and any other assets to assets directory here, > > instead of removing clear parameter, this will avoid potential issue in the > > future. > > Done. But the clear flag makes issue like this very difficult to figure out. Thanks, using 'clear': 1 prevents unused asset files from packaging to apk, we should always enable it.
On 2016/03/09 19:39:02, AndyWu wrote: > https://codereview.chromium.org/1776933002/diff/1/chromecast/chromecast.gyp > File chromecast/chromecast.gyp (left): > > https://codereview.chromium.org/1776933002/diff/1/chromecast/chromecast.gyp#o... > chromecast/chromecast.gyp:582: 'clear': 1, > On 2016/03/09 17:56:09, michaelbai wrote: > > You should copy cast_shell.pak and any other assets to assets directory here, > > instead of removing clear parameter, this will avoid potential issue in the > > future. > > Done. But the clear flag makes issue like this very difficult to figure out. Thanks, using 'clear': 1 prevents unused asset files from packaging to apk, we should always enable it.
lgtm
The CQ bit was checked by tsunghung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1776933002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1776933002/40001
The CQ bit was unchecked by tsunghung@chromium.org
The CQ bit was checked by tsunghung@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from halliwell@chromium.org, sanfin@chromium.org Link to the patchset: https://codereview.chromium.org/1776933002/#ps40001 (title: " ")
The CQ bit was unchecked by tsunghung@chromium.org
The CQ bit was checked by tsunghung@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1776933002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1776933002/40001
Message was sent while issue was closed.
Description was changed from ========== Add the missing asset file cast_shell.pak back All files under assets directory will be removed when building cast_shell_icudata target. We have to make sure all the asset files must be collected here. BUG=593277 ========== to ========== Add the missing asset file cast_shell.pak back All files under assets directory will be removed when building cast_shell_icudata target. We have to make sure all the asset files must be collected here. BUG=593277 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add the missing asset file cast_shell.pak back All files under assets directory will be removed when building cast_shell_icudata target. We have to make sure all the asset files must be collected here. BUG=593277 ========== to ========== Add the missing asset file cast_shell.pak back All files under assets directory will be removed when building cast_shell_icudata target. We have to make sure all the asset files must be collected here. BUG=593277 Committed: https://crrev.com/c058232e6063829ab7db5dab45bdbdbb988b06d0 Cr-Commit-Position: refs/heads/master@{#380202} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c058232e6063829ab7db5dab45bdbdbb988b06d0 Cr-Commit-Position: refs/heads/master@{#380202}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1782133003/ by alokp@chromium.org. The reason for reverting is: Break chromecast gyp build: "unknown target 'assets/cast_shell.pak'". |
